-
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
chore: add PR testing workflows #6
Conversation
abfef8e
to
7e63ea0
Compare
497a501
to
4b9307f
Compare
let db_password_raw = kms::decrypt(kms_client, "DB_PASSWORD").await; | ||
let db_password = encode(db_password_raw.as_str()).to_string(); | ||
let app_env: String = get_from_env_or_default("APP_ENV", "TEST".into()); | ||
let db_password: String = if app_env.as_str() == "TEST" { |
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.
any reason to deviate from the KMS setup for testing?
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.
Why do we do this in development environments? Makes easy setups complex. So I wanted to add in an exception for this. Also, KMS encryption is only for juspay, we should remove this from this repo
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.
Sure, make sense.
make superposition -e DOCKER_DNS=$(DOCKER_DNS) | ||
|
||
ci-test: cleanup ci-setup |
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.
why remove cleanup from here?
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.
We don't need docker cleanup anymore, github actions automatically cleans up containers
@@ -1,4 +1,4 @@ | |||
DATABASE_URL=postgres://postgres:docker@dockerdns:5432/config?sslmode=disable | |||
DATABASE_URL=postgres://postgres:docker@localhost:5432/config?sslmode=disable |
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.
changing from dockerdns
to localhost
as we won't have docker-in-docker, since we are moving away from Jenkins?
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.
Yup
68aac6f
to
fdb6177
Compare
48777d4
to
228adb3
Compare
Problem
Superposition doesn't have testing for every PR setup
Solution
add that using github actions