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

feat(controller): optional database migration #4869

Merged
merged 1 commit into from Jan 15, 2021
Merged

Conversation

book987
Copy link
Contributor

@book987 book987 commented Jan 13, 2021

#4822

Checklist:

@CLAassistant
Copy link

CLAassistant commented Jan 13, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

My thought on this is - how do users know what SQL needs to be applied? Could we modify migrate.go to with a print-only mode - i.e. print out the SQL statements, but not apply them?

workflow/controller/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
@book987
Copy link
Contributor Author

book987 commented Jan 15, 2021

My thought on this is - how do users know what SQL needs to be applied? Could we modify migrate.go to with a print-only mode - i.e. print out the SQL statements, but not apply them?

🤔 I think print out mode is

  1. helpful when using Argo at first time, just get the output and apply somewhere
  2. but help little or useless when upgrading Argo, most users don't want to drop and rebuild database i think, they need to find a way generating what they should apply. Even tracking changes from commits since last version is more efficent.
  3. also, user should run another kubectl command to get what we print out, and filter SQL from all output by them self

And, like i mention at #4822, i don't think hardcoded db migration in go source code is a good way. I think we can:

  • use a seperated directory for db migration like Django do, new schema changes means new migration file, user can get schema chagnes by just seeking at migration directory
  • or we can just provide a SQL file represent schema changes when releasing new version

But the ways i mention above is not friendly for user using Argo at first time, Maybe we needs further disign.

Signed-off-by: book987 <book78987book@gmail.com>
@alexec alexec added this to the v3.0 milestone Jan 15, 2021
@alexec
Copy link
Contributor

alexec commented Jan 15, 2021

I think this is plenty good enough for MVP.

@alexec alexec merged commit dd8c1ba into argoproj:master Jan 15, 2021
@simster7 simster7 mentioned this pull request Jan 19, 2021
17 tasks
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