-
Notifications
You must be signed in to change notification settings - Fork 43
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
MM-61944: Decouple vacuum step from Terraform.Create and use it in Comparison #853
Conversation
This way we run the vacuum step when we need it: *after* we load the database dump. We can also remove the duplicated clearLicensesCmd, since that's also ran in the post-processing step.
Actually, @fmartingr, can you take a look as well? Specially to the extra SQL commands stuff, since that's something you have more context on. The ordering of steps is still correct, right? |
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.
LGTM. I think that at some point we need to get the InitData
value from the deployer.json or a flag.
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.
Thanks! I agree it's messy and could use some more love.
@@ -28,12 +29,20 @@ func RunCreateCmdF(cmd *cobra.Command, args []string) error { | |||
return fmt.Errorf("failed to create terraform engine: %w", err) | |||
} | |||
|
|||
initData := config.DBDumpURI == "" | |||
err = t.Create(initData) | |||
extAgent, err := ssh.NewAgent() |
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.
Curious, is there any advantage in passing this agent around vs creating it inside these methods (Create
and PostProcessDatabase
)?
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.
Not really, to be honest, apart from the code duplication. I was in refactor mode, so duplicated code I saw, duplicated code I removed 😂
Thanks Felipe and Claudio for taking care of the review! |
They were quick! |
Summary
Last week, @streamer45 realized that we're not running the
vacuum
step in comparison runs. Actually, we do run it, but we do it before loading the database dump, which is of course completely useless.This PR refactors that step (and the rest of post-processing we do after the DB is ready: clear licenses data and run extra SQL commands) out from
Terraform.Create
, so we can call it when needed in both usages of that function: in the usualdeployment create
command and in thecomparison run
command.There's still a lot of work to decouple everything going on in
Terraform.Create
, but this is a baby step in that direction.Ticket Link
https://mattermost.atlassian.net/browse/MM-61944