-
Notifications
You must be signed in to change notification settings - Fork 18
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
registry: sync staging and dev environments with prod's DB and S3 #704
Conversation
This pull request is automatically being deployed by Amplify Hosting (learn more). |
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.
Coupla nits, but at a high level could you add some documentation to explain what all this is for, why it exists, how it works?
@@ -27,6 +28,7 @@ impl Default for Config { | |||
auth_token: from_env_default("AUTH_TOKEN", "").parse().unwrap(), | |||
clerk_secret_key: env::var("CLERK_SECRET_KEY").expect("CLERK_SECRET_KEY not set"), | |||
github_token: env::var("GITHUB_TOKEN").expect("GITHUB_TOKEN not set"), | |||
environment: from_env_default("ENV", "dev").parse().unwrap(), |
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.
I suggest using TRUNK_ENV
, as ENV
is super generic and may have other uses. Uh, unless its supposed to be and all of our services would use it the same way?
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.
@sjmiller609 What do you think? I guess it depends on how this is setup in app-deploy
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.
I agree with David
Yes sir! The main idea is that our current dev and staging environments lack sufficient content and are not similar enough to our production Trunk environment. As a result, our dev and staging Tembo Cloud environments currently rely on prod Trunk, making it harder to detect issues in dev Trunk. Once this PR achieves its goal (having dev and staging contain mostly the same content as production), we can have each Tembo Cloud environment use its respective Trunk, as outlined in CLOUD-739 |
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.
Looks good, but unless this is a one-off thing, I'd really like to see tests.
if s.eq_ignore_ascii_case("prod") { | ||
Ok(Env::Production) | ||
} else if s.eq_ignore_ascii_case("staging") { | ||
Ok(Env::Staging) | ||
} else if s.eq_ignore_ascii_case("dev") { | ||
Ok(Env::Development) | ||
} else { | ||
Err("Invalid value: expected 'prod', 'staging' or 'dev'") | ||
} |
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.
Wondering if a match
statement would be clearer:
match s.to_ascii_lowercase() {
"prod" => Ok(Env::Production),
"staging" => Ok(Env::Staging)
"development" => Ok(Env::Development)
_ => Err("Invalid value: expected 'prod', 'staging' or 'dev'")
}
No description provided.