Skip to content

DBPW - Rename newdbplugin package to dbplugin/v5#10151

Merged
pcman312 merged 11 commits into
masterfrom
dbpw-package-rename
Oct 15, 2020
Merged

DBPW - Rename newdbplugin package to dbplugin/v5#10151
pcman312 merged 11 commits into
masterfrom
dbpw-package-rename

Conversation

@pcman312
Copy link
Copy Markdown
Contributor

@pcman312 pcman312 commented Oct 14, 2020

Package renaming - Part 1/4

This is the first in a 4 part series of changes to rename the sdk/database/newdbplugin package to sdk/database/dbplugin/v5.

  1. Make copy of the newdbplugin package to dbplugin/v5 so external/vendored DB plugins can reference it (and not break the current build)
  2. Update external repos to the new package name
  3. Vendor external repos into Vault
  4. Remove the newdbplugin package
  5. ???
  6. Profit

Overview

  • Copies the newdbplugin package under dbplugin as sdk/database/dbplugin/v5.
  • Moves the proto files under sdk/database/dbplugin/v5/proto so it doesn't keep multiple copies of the same proto (which causes registry collisions)
  • Updates references in the DB engine to the dbplugin/v5 package.

Description

The sdk/database/newdbplugin package name was always intended to be a temporary name during development with the thought that it would replace the existing sdk/database/dbplugin package (with that package being renamed out of the way). That plan changed to moving newdbplugin under the sdk/database/dbplugin folder.

This keeps a copy of the original newdbplugin package (with one exception) in place because the package is being referenced in at least one vendored DB plugin (couchbase) causing build errors. Keeping it in place allows us to migrate the external/vendored DB plugins over to the new package seamlessly. The one exception is the protos sub-package. This was moved from newdbplugin to dbplugin/v5 and a copy was not kept in the original newdbplugin package. This was done so the protobuf registry didn't see two copies of the same protos. All references to the proto have been updated to the new location.

Testing

I tested this also against a set of bats scripts I have that are doing black box tests against Vault for: Cassandra, InfluxDB, MongoDB, MSSQL, PostgreSQL, and an external mock v4 database plugin.

PRs

Part 2:
hashicorp/vault-plugin-database-couchbase#12
hashicorp/vault-plugin-database-elasticsearch#21
hashicorp/vault-plugin-database-mongodbatlas#16
Part 3:
#10188
Part 4:
#10216

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.

Looks good, except for one extraneous file?

Comment thread plugins/database/cassandra/testoutput.json Outdated
MaxTTL time.Duration `json:"max_ttl"`
StaticAccount *staticAccount `json:"static_account" mapstructure:"static_account"`
DBName string `json:"db_name"`
Statements v4.Statements `json:"statements"`
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.

Not specific to this PR, but noticed that we're still using a v4 struct for storing the statements even though there are v5 equivalents of these. We'll probably have to handle upgrades if we wanted to fully migrate out of v4.

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.

We can certainly look into it, though I suspect we'll end up keeping the same or very similar struct definition for these since the list of statements hasn't changed and still needs to be stored.

@pcman312 pcman312 merged commit a69ee0f into master Oct 15, 2020
@pcman312 pcman312 deleted the dbpw-package-rename branch October 15, 2020 19:20
pcman312 added a commit that referenced this pull request Oct 20, 2020
This is part 1 of 4 for renaming the `newdbplugin` package. This copies the existing package to the new location but keeps the current one in place so we can migrate the existing references over more easily.
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