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

Configs/env vars: s/OPT_/ABC_. #565

Merged
merged 4 commits into from
Jun 15, 2022
Merged

Conversation

webbhorn
Copy link
Contributor

@webbhorn webbhorn commented Jun 8, 2022

Previously we had lots of environment variables prefixed with OPT
as a holdover from the Optics fork. This PR tries to migrate them to
Abacus-specific prefix of ABC, which is something we'd said we'd like
to do prior to handing operational responsibility for running agents to
any third parties, since there will be some inherent resistance in
persuading a group like that to rename all configuration variables.

If desired, I'm happy to hold on this since we have so much in-flight
at the moment, just sending out for initial comment. On the other hand,
maybe it's better to catch and fix any issues as soon as we can.

This was a fairly blind find-and-replace for OPT_ and OPT strings within
the repo. I'm less familiar with the agent deploy/release process so am
leaning to some extent on @tkporter / @nambrot to comment on whether
I've managed to miss any of the critical config-generating pieces of code.

Previously we had lots of environment variables prefixed with OPT
as a holdover from the Optics fork. This PR tries to migrate them to
Abacus-specific prefix of ABC, which is something we'd like to do
prior to handing operational responsibility for running agents to
any third parties, since there will be some inherent resistance in
persuading a group like that to rename all configuration variables.
Copy link
Contributor

@nambrot nambrot left a comment

Choose a reason for hiding this comment

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

lgtm, though I would defer to @tkporter whether external-secrets can automatically update it's secrets it manages. I believe yes

@webbhorn
Copy link
Contributor Author

Thanks, I'll wait for Trevor.

@webbhorn
Copy link
Contributor Author

Resolved merge conflict (new whitelist-related env var for relayer in run-locally.sh), verified still works by running run-locally.sh and watching logs.

Copy link
Collaborator

@tkporter tkporter left a comment

Choose a reason for hiding this comment

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

lgtm! I just deployed using a new image so we don't get into a situation where we accidentally deploy using an image that expects OPT_* but ABC_* is supplied.

@tkporter tkporter enabled auto-merge (squash) June 15, 2022 10:12
@tkporter tkporter merged commit 3ec86b0 into main Jun 15, 2022
@tkporter tkporter deleted the webbhorn/s-slash-opt-slash-abc branch June 15, 2022 10:18
@webbhorn
Copy link
Contributor Author

Thanks!

yorhodes pushed a commit that referenced this pull request Jun 29, 2022
* Transition to Abacus-themed env var prefix.

Previously we had lots of environment variables prefixed with OPT
as a holdover from the Optics fork. This PR tries to migrate them to
Abacus-specific prefix of ABC, which is something we'd like to do
prior to handing operational responsibility for running agents to
any third parties, since there will be some inherent resistance in
persuading a group like that to rename all configuration variables.

* Update image to use ABC_ instead of OPT_, rm --debug --dry-run flags from agent helm command

Co-authored-by: Trevor Porter <trkporter@ucdavis.edu>
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.

None yet

3 participants