-
Notifications
You must be signed in to change notification settings - Fork 97
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
[ENH] - Use GitHub secrets instead of Vault #2889
[ENH] - Use GitHub secrets instead of Vault #2889
Conversation
Thanks @smokestacklightnin, this looks good! We'll wait for @dcmcand to come back so we can add the secrets and test the workflows. I think the provider workflow won't run from a fork and the other three (aws, azure and gcp) will need to be triggered manually. |
The AWS Deployment test failure shows a puzzling error (I included line numbers because it is a very long file):
I'm not sure yet why the region isn't being recognized when the region is specified in at least one place. CC: @marcelovilla |
@smokestacklightnin I ran the tests again with a less verbose Terraform log level, and found the following issue:
We need to make sure that secret is exposed as an environment variable too. I can make that change |
@smokestacklightnin I exposed
I'm still waiting until we fix #2893 to run the Azure one. Once that is done, I think we're ready to merge either of the PRs. If we're merging this, please merge my branch into it as it has the latest changes. |
Azure tests are passing: https://github.com/nebari-dev/nebari/actions/runs/12813193533 |
…se-github-secrets-instead-of-vault
d07ecbb
to
34ab5dd
Compare
…ecrets-instead-of-vault' into ci/authentication/use-github-secrets-instead-of-vault
34ab5dd
to
1d161ce
Compare
@smokestacklightnin can you include this commit in this PR? While everything was already working fine, I noticed we had a duplicate env variable and decided to expose some of the env vars at the step level instead of the workflow level. Tested from my PR and everything looks good. |
@marcelovilla Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @smokestacklightnin 🚀
Reference Issues or PRs
Resolves #2835
What does this implement/fix?
Put a
x
in the boxes that applyTesting
How to test this PR?
Any other comments?