Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DCJ-472: Migrate render-configs.sh to use GSM #1739

Merged
merged 6 commits into from
Jul 17, 2024

Conversation

fboulnois
Copy link
Contributor

@fboulnois fboulnois commented Jul 16, 2024

Addresses

https://broadworkbench.atlassian.net/browse/DCJ-472

Summary of changes

Switches the default secret manager to GSM, using @pshapiro4broad 's existing branch as a base.

Testing Strategy

Compared Vault and GSM outputs in VScode.

@fboulnois fboulnois requested review from a team as code owners July 16, 2024 17:29
@fboulnois fboulnois requested review from rushtong and aarohinadkarni and removed request for a team July 16, 2024 17:29
Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

👍🏽

Copy link
Contributor

@okotsopoulos okotsopoulos left a comment

Choose a reason for hiding this comment

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

Thank you! Off of a freshly restarted computer, I pulled your feature branch down, ran this script with the inputs suggested in its documentation, and was successfully able to ./gradlew bootRun TDR.

Both of my suggestions can be considered optional.

AZURE_ENV=dev
RBS_ENV=tools
COPY_INTELLIJ_ENV_VARS=n
USE_VAULT="${USE_VAULT:-false}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I like leaving this backdoor to Vault for the time being. Should we also include it in the script documentation above?

Better yet, I think this script would also benefit from a usage command like you added for the DB connect script. But it would be fair to call that improvement OOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially had this in, but I think we should not encourage the use of Vault through a CLI argument, hence why I removed it. Relatedly my idea was once this change was merged and we were comfortable with GSM then I would make a separate PR that deprecates Vault completely and removes this and the other codepaths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created https://broadworkbench.atlassian.net/browse/DCJ-526 to address these items.

@@ -19,9 +19,12 @@
# Then, refresh your z-shell configuration (`source ~./zshrc`)
Copy link
Contributor

Choose a reason for hiding this comment

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

(Can be considered OOS -- previously existing documentation bug.)

I think this z-shell command and the one on line 13 are incorrect: for me, it works as source ~/.zshrc. If you'd like to fix these while in the neighborhood, please feel free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll say this one is out-of-scope for now, but in general these instructions make some assumptions about how the developer has set up their shell (e.g. I use Bash, not ZSH) which I'm not super comfortable with. Perhaps worth reviewing when we fully deprecate the Vault codepath?

@fboulnois fboulnois merged commit 68940ce into develop Jul 17, 2024
5 checks passed
@fboulnois fboulnois deleted the ps/dcj-406-vault-migration branch July 17, 2024 16:55
Shakespeared pushed a commit that referenced this pull request Jul 23, 2024
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.

4 participants