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

Run postupgrade job only when upgrading #1760

Merged
merged 2 commits into from Jun 3, 2020
Merged

Run postupgrade job only when upgrading #1760

merged 2 commits into from Jun 3, 2020

Conversation

andresmgot
Copy link
Contributor

Description of the change

This job creates the db secret (if it hasn't been created at installation time) and cleans up the database. As far as I can tell none of these processes are necessary when installing the app. Related to #1754

@andresmgot
Copy link
Contributor Author

it's failing for MongoDB so I am probably forgetting something

@absoludity
Copy link
Contributor

Postgres handles this because the Sync() command calls InitTables as postgres supports create table if it doesn't exist, whereas currently we'd need to update dbutils.mongodbutils with something that ensures the indicies exist. That said, I think we should move in the opposite direction, so that when we are pg-only, we always run the migration post-install/post-upgrade ?

How is this change helping #1754, given both post-install or post-upgrade are run after all resources are created?

@andresmgot
Copy link
Contributor Author

That said, I think we should move in the opposite direction, so that when we are pg-only, we always run the migration post-install/post-upgrade ?

Why would be the migration needed in the first install? (maybe I am missing something).

How is this change helping #1754, given both post-install or post-upgrade are run after all resources are created?

Is post-upgrade run after installing a chart? That would be weird? My point here was that if the job running the postupgrade action fails, the installation fails while this seemed to me like a step we can skip.

@andresmgot
Copy link
Contributor Author

as talked in person, we agreed on initializing tables and collections in the first sync (if they don't exist) so we don't need to run the upgrade job when installing the chart

Merging this PR to cut a release, let me know if you find any issue and I will open a follow up PR.

@andresmgot andresmgot merged commit ea22723 into master Jun 3, 2020
@andresmgot andresmgot deleted the postUpgrade branch June 3, 2020 10:16
@absoludity
Copy link
Contributor

absoludity commented Jun 3, 2020 via email

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

2 participants