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

use values.yaml when testing for postgres readiness #1143

Merged
merged 2 commits into from Jun 19, 2021

Conversation

ajsalow
Copy link
Contributor

@ajsalow ajsalow commented Jun 15, 2021

Signed-off-by: ajsalow ajsalow@umich.edu

Close #1142

@welcome
Copy link

welcome bot commented Jun 15, 2021

Thank you for opening this pull request! 🙌
These tips will help get your PR across the finish line: - Most of the repos have a PR template; if not, fill it out to the best of your knowledge. - Sign off your commits (Reference: DCO Guide).

kumare3
kumare3 previously approved these changes Jun 15, 2021
Copy link
Contributor

@kumare3 kumare3 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

@@ -24,7 +24,7 @@ spec:
command:
- sh
- -c
- until pg_isready -h postgres -p 5432; do echo waiting for database; sleep 2; done;
- until pg_isready -h {{ .Values.db.datbase.host }} -p {{ .Values.db.database.port }}; do echo waiting for database; sleep 2; done;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit : Values.db.database.host

@ajsalow
Copy link
Contributor Author

ajsalow commented Jun 15, 2021

Looks like the test is failing because neither values-gcp.yaml or values.yaml specify a database config. This causes the helm output to have until pg_isready -h -p ; do echo waiting for database; sleep 2; done; instead of until pg_isready -h postgres -p 5432; do echo waiting for database; sleep 2; done;. Let me know if you want me to update one of values files to contain the default db config.

@sbrunk
Copy link
Member

sbrunk commented Jun 15, 2021

@ajsalow the test fails because you need to run make helm and push the generated manifests to git.

Background:
make helm runs https://github.com/flyteorg/flyte/blob/master/script/generate_helm.sh which will generate manifests for
values-sandbox.yaml, values-aws.yaml and values-gcp.yaml (not for the default values.yaml).
Don't worry about the broken gcp config, it's not finished yet.

@sbrunk sbrunk added the helm label Jun 15, 2021
@sbrunk
Copy link
Member

sbrunk commented Jun 16, 2021

Sorry @ajsalow could you rebase against master and re-run make helm to resolve the conflict? Also could you make sure to use helm 3.6 because it formats some lines diffently, otherwise the test is going to fail again.

Signed-off-by: ajsalow <ajsalow@umich.edu>
Signed-off-by: ajsalow <ajsalow@umich.edu>
@ajsalow ajsalow force-pushed the 1142-flyteadmin-stuck-waiting branch 2 times, most recently from d5e860b to 45aa4f8 Compare June 18, 2021 15:24
@ajsalow
Copy link
Contributor Author

ajsalow commented Jun 18, 2021

@sbrunk Thanks for the help. Hopefully it is good to go now!

Copy link
Member

@sbrunk sbrunk left a comment

Choose a reason for hiding this comment

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

Looks good to me. LGTM!

@kumare3 kumare3 merged commit 5114b4f into flyteorg:master Jun 19, 2021
@welcome
Copy link

welcome bot commented Jun 19, 2021

Congrats on merging your first pull request! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flyteadmin gets stuck in waiting if db host or port changes.
4 participants