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

MM-61944: Decouple vacuum step from Terraform.Create and use it in Comparison #853

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

agarciamontoro
Copy link
Member

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 usual deployment create command and in the comparison 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

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.
@agarciamontoro agarciamontoro added the 2: Dev Review Requires review by a core committer label Nov 25, 2024
@agarciamontoro
Copy link
Member Author

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?

Copy link
Contributor

@fmartingr fmartingr left a 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.

Copy link
Contributor

@streamer45 streamer45 left a 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()
Copy link
Contributor

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)?

Copy link
Member Author

@agarciamontoro agarciamontoro Nov 26, 2024

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 😂

@agarciamontoro agarciamontoro removed the request for review from agnivade November 26, 2024 08:32
@agarciamontoro agarciamontoro added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Nov 26, 2024
@agarciamontoro agarciamontoro merged commit 8ff7d7b into master Nov 26, 2024
1 check passed
@agarciamontoro agarciamontoro deleted the MM-61944.decouple.vacuum branch November 26, 2024 08:32
@agnivade
Copy link
Member

Thanks Felipe and Claudio for taking care of the review!

@agarciamontoro
Copy link
Member Author

They were quick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants