Skip to content

DBPW - Update Cassandra to adhere to v5 Database interface#10051

Merged
pcman312 merged 6 commits into
masterfrom
dbpw-cassandra
Oct 12, 2020
Merged

DBPW - Update Cassandra to adhere to v5 Database interface#10051
pcman312 merged 6 commits into
masterfrom
dbpw-cassandra

Conversation

@pcman312
Copy link
Copy Markdown
Contributor

@pcman312 pcman312 commented Sep 28, 2020

Overview

Updates the Cassandra Database plugin to adhere to the v5 Database interface.

Prerequisites

Base automatically changed from dbpw-usernames to master September 29, 2020 22:54
Comment thread plugins/database/cassandra/cassandra.go Outdated
m := map[string]string{
"username": req.Username,
}
err := session.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is purely stylistic, but I don't think we tend to break chained function calls into multiple lines unless that's broken out by a multi-line struct declaration in the function's input param. Breaking out object's instantiation field values into multiple lines, like with m above, is fine though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up doing this because the line was getting fairly long with a lot of parenthesis. I can change it back if you want though.


c.rawConfig["password"] = password
return c.rawConfig, nil
return newdbplugin.DeleteUserResponse{}, result.ErrorOrNil()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to ask this offline earlier, but can you remind me why the interface doesn't return pointer response values so that we can return nil?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't a particular need to do so. Plus it opens up the problem of having to do nil checks in other parts of the code and having some potentially inconsistent or confusing behavior.

@pcman312 pcman312 requested a review from calvn October 7, 2020 19:57
Copy link
Copy Markdown
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 plus a couple questions.

Comment thread plugins/database/cassandra/cassandra.go
err = db.RevokeUser(context.Background(), statements, username)
ctx, cancel = context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
_, err = db.DeleteUser(context.Background(), deleteReq)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_, err = db.DeleteUser(context.Background(), deleteReq)
_, err = db.DeleteUser(ctx, deleteReq)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

func testCredsExist(address string, port int, username, password string) error {
func assertNoCreds(t testing.TB, address string, port int, username, password string) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why assertCreds() needs a retry with backoff, but assertNoCreds() doesn't.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I neglected to put it in assertNoCreds.

Comment thread plugins/database/cassandra/cassandra_test.go Outdated
@pcman312 pcman312 merged commit 1eff3f7 into master Oct 12, 2020
@pcman312 pcman312 deleted the dbpw-cassandra branch October 12, 2020 21:54
pull Bot pushed a commit to mayocream/vault that referenced this pull request Oct 10, 2025
* update skipped tests and remove an unused test

* update how event is triggered to reduce flakiness

* remove test

* Revert "remove test"

This reverts commit 8d166994fb83f2030ac7b50ef125a48f030b80d3.

* wait for tooltip to render

* remove test

Co-authored-by: lane-wetmore <lane.wetmore@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants