Add support for labels/annotations and disabling on update job

[mjnagel]

Summary

This adds a new struct and three new "options" to the MM spec:

spec:
  updateJob:
    extraLabels: {}
    extraAnnotations: {}
    disabled: (bool)

The annotations/labels will be added to the update job pod, but will not override the default app label set on the job pod.

disabled will control the creation/use of the update job, defaulting to the same behavior as previously.

Also adds tests for each of these new pieces.

Ticket Link

Fixes https://github.com/mattermost/mattermost-operator/issues/290

Release Note

Add support for labels and annotations to be specified for update job pod
Add ability to disable the update job pod for advanced configuration

[mattermod]

Hello @mjnagel,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

[mjnagel]

@Szymongib leaving this in draft right now because I have two questions regarding it...

1. Clusterinstallation uses the same functions for the update job. Since these changes require adding a new parameter to pass these update functions part of the MM spec I needed to address the function calls from clusterinstallation code. I'm not sure if the way I approached it is the best, but it effectively passes in an empty struct to fulfill the required params. Happy to alter this if there's a better way to approach - only other thing I could think of was make separate functions for each which isn't very DRY.
2. Part of the issue I opened (https://github.com/mattermost/mattermost-operator/issues/290) was the goal of allowing a modification/extension of the command this job runs. My use case would be to run an istio sidecar termination script after the main mattermost version command has run. I imagine others might have similar desire to run something after the upgrade, but tied to the lifecycle of the update job. If this is something you'd be open to supporting what would be the best way to approach it? Allow a full override of the command (spec.updateJob.command), which would be the most flexible but open end users to deleting the main upgrade functionality? Alternatively we could allow for something that would be appended to/run after the default command (spec.updateJob.postUpgradeCommand) which would fit most needs I could think of?

[Szymongib]

Thanks for another PR @mjnagel.

1. I think passing empty structs as arguments for the ClusterInstallation code would be fine since this will not alter the behavior.
2. I am still not sure what is the best way to handle overriding the update job command. I think that the simplest approach would be just to give the ability to disable it. It does not have any special behavior rather than failing if the image is incorrect so if it is disabled, new pods just won't come up therefore update will fail but the old pods should remain running.

[mjnagel]

@Szymongib appreciate the feedback, I believe I've implemented a disable value in a simple way that should work (and added some tests around all of this new functionality). Let me know if any changes are needed, hopefully this is good to go 😄

---

I do want to add a comment to one thing you mentioned...

It does not have any special behavior rather than failing if the image is incorrect

In our usage the update job has been extremely helpful for the DB migrations - if I'm not mistaken it will perform them on startup (and I saw a comment in the code to that effect as well). It's not super special (no magical code doing this that we couldn't implement), but it is nice to have this all handled by the operator and tied into the lifecycle in the right order.

I'm good with the disabled solution for now since it provides a path forward with our primary use case for istio injection, but might be something to give some more thought to in the future.

[Szymongib]

In our usage the update job has been extremely helpful for the DB migrations

DB migrations should run auto automatically when the new instance start. That what I was referring to about job not really doing any magic, sorry, I could have been a bit cleaner on that.

[gabrieljackson]

Looks like there is a merge conflict as expected from the related PR. If you could resolve that we will get this merged asap. Thanks!

[mjnagel]

@gabrieljackson / @Szymongib should be good to go? Rebased and fixed those comments - appreciate the feedback and assistance 😄

[gabrieljackson]

Going to request one more review from Szymon to look over the final code.