Adding rollout strategy variable for Flux deployment in chart

[ViBiOh]

This PR adds the ability to configure the rollout strategy for the flux deployment. This aims to be a non-breaking change as if configuration not provided, nothing changed.

This PR enforces the rollout strategy to Recreate for flux, in order to avoid two running instance when performing a rollout.

[squaremo]

I'm curious about the use case you have in mind. In general you don't want or need any more than one flux pod of a particular deployment running; so Recreate is the only sensible strategy (I'm surprised it's not just inlined into the chart).

[ViBiOh]

I'm curious about the use case you have in mind. In general you don't want or need any more than one flux pod of a particular deployment running; so Recreate is the only sensible strategy (I'm surprised it's not just inlined into the chart).

Yep exactly, I want the Recreate strategy but as there is no strategy defined, the default strategy is RollingUpdate which cause to have 2 pods running at the same time.

I didn't want to enforce the strategy or break existing behavior for others users, so I've setted it under condition.

[squaremo]

I didn't want to enforce the strategy or break existing behavior

Ah I see, you were being careful -- I shan't argue with that :-)

I expect people do not notice that it's defaulting to rolling update, and would not notice a change. Previously there was no way to override the strategy, and there is no reason to run more than one replica. I would just hard-wire the strategy to Recreate.

[ViBiOh]

I would just hard-wire the strategy to Recreate.

I've updated my pull-request (and the associated comment) to reflect this ;)

[squaremo]

Great! To pass the DCO check, follow these instructions: https://github.com/fluxcd/flux/pull/3325/checks?check_run_id=1475249188. If you are happy rebasing, rebasing on master from this repo will remove the "out-of-date" message too (but I can do that rebase if you're not comfortable doing it).

[ViBiOh]

Great! To pass the DCO check, follow these instructions: https://github.com/fluxcd/flux/pull/3325/checks?check_run_id=1475249188. If you are happy rebasing, rebasing on master from this repo will remove the "out-of-date" message too (but I can do that rebase if you're not comfortable doing it).

Rebased and signoff =) Should I bump the chart's version (I think it's a minor, new feature but not a breaking one, but can be seen as a fix too) or it's automatic with the Continuous Integration ?

[squaremo]

Should I bump the chart's version (I think it's a minor, new feature but not a breaking one, but can be seen as a fix too) or it's automatic with the Continuous Integration ?

It's done by a human when the new chart is released and published -- good thinking, but you don't need to do it.

[ViBiOh]

Hi 👋 What's the next step I need to do on this pull-request to get a review ?

[yebyen]

Hi @ViBiOh, it looks like a small change and already discussed with @squaremo

I see master has already moved again twice since your last rebase, if you can rebase again that will help when it's approved and ready to merge to save a step. Hoping to get more time on this soon myself! I am a friend of flux but do not have my contributor badge yet.

I'm not acquainted with the release side for Flux v1, but like any release there is overhead and we'd like to have fewer releases on the books from now with Flux v1 in maintenance mode, I'm looking through issues and yours looks like one to merge and include in the next release.

I suspect since it's small and already signed off, it can be put into a release. I understand and sympathize with your desire to not maintain a separate fork of Flux! I see you've already rebased multiple times, sorry to ask you to have to do it again.

[ViBiOh]

I understand and sympathize with your desire to not maintain a separate fork of Flux! I see you've already rebased multiple times, sorry to ask you to have to do it again.

I've rebased it on master. It's more to have the benefit of my changes and to clear a bit more my "opened pull-request" list =)

[kingdonb]

I'm presently reviewing all the pull requests and will present this one at the next opportunity as "recommended for merge"

To clarify, I have just tested the upgrade by hand with helm upgrade to be sure about the behavior during upgrade, since the default RollingUpdate strategy includes a rollingUpdate: section that is not permitted with Recreate strategy, I wanted to be sure that Helm will handle it better than "kubectl edit" that balks at deployment validation – the helm upgrade went well, so – "recommended for merge"

[kingdonb]

I included this in #3421, with a note that closes this PR.

If #3421 is approved by maintainers, it will merge and this PR will have been included in the next release. 👍

[ViBiOh]

I included this in #3421, with a note that closes this PR.

If #3421 is approved by maintainers, it will merge and this PR will have been included in the next release. 👍

Should I rebase my branch on latest commit of the default branch?

[kingdonb]

No need, I collected all the PRs that I reviewed and approved for (#3421) which is a single PR for the maintainers to approve for the release. Your change from this PR is included there. (This is slightly irregular as you will note I am preparing the release but I am not yet a maintainer.)

I am not sure how many more Flux v1 releases there will be from now on, but I like this strategy as a non-maintainer it allows me to organize the release better. The aim is to divert as little of core Flux maintainers time as possible while they continue to focus mainly on delivering new features and new releases in Flux v2.

[kingdonb]

Closing this manually, it was included via #3421 in the 1.21.2 release that was cut just now.

(Chart upgrade incoming to follow testing.) 👍 Thanks for your contribution