Skip to content

Conversation

@ciaransweet
Copy link
Contributor

What this PR is

This PR parameterises the db_snapshot_admin_username parameter. This is because the create_database_server function internally was hardcoding the admin username of snapshots, which we discovered didn't align to our example in CSDA STAC API: https://github.com/NASA-IMPACT/csda-project/issues/753#issuecomment-2504708053

With this change, we can now override this value so that when a DB is restored from a snapshot, its admin user just gets a new password generated and we don't lock ourselves out 🎉

As this is a breaking change (changed the default value to be the RDS default value and parameterised it) I've bumped the version - Not sure how we release this, but looks like we just install via git url in the projects that use this.

What I changed

  • Bumped version in VERSION
  • Added in .flake8 just to reduce some linting issues (I envision in another workstream we actally give this repo some TLC in terms of dev tooling)
  • Added a new example to the README.md for the above changes and removed some unused vars in the other examples
  • Lint fixes in cdk_bootstrapped_db/constructs.py
  • Parameterised db_snapshot_admin_username in cdk_boostrapped_db/helpers.py:create_database_server

How you can test this

If you're using the create_database_server functionality with a snapshot, you should be able to install this new version and just provide db_snapshot_admin_username as the value of the admin user of the snapshot and you should get a cdk diff with no changes to that construct.

Or just watch this issue as we tackle it: https://github.com/NASA-IMPACT/csda-project/issues/753

ceholden and others added 2 commits November 27, 2024 13:13
* Also removes some unused variables + imports
* Update readme with example
* Bump version
@ciaransweet
Copy link
Contributor Author

FWIW I did some searching and I don't even think (Looking at current RDS deployments and CDK usages in the NASA-IMPACT org) that the superuser value was ever valid/used. So this might not actually constitute as 'breaking' in the sense of someone will have a bad time right now if they swapped, but hey ho. Good practice to treat it as breaking anyway.

Copy link
Contributor

@ceholden ceholden left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for reviewing the other uses of this! +1 on bumping the VERSION, that's my understanding of how to cut a new release and +1 for major bump for breaking change

@ciaransweet ciaransweet merged commit 9ffd759 into main Dec 2, 2024
@ceholden ceholden deleted the ceh/fix-snapshot-username branch December 2, 2024 16:37
@ceholden
Copy link
Contributor

ceholden commented Dec 2, 2024

@ciaransweet I was fuzzy on the release process here (there's no action for deploy/release) but this PR looked good and correct. I'll create a new tagged release for this so we can reference it via the version number, and then I can point to it from our services

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants