feat!(logging): Always archive logs if in config. Closes #1790

[alexec]

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234". Why? for the release notes.
• Optional. I've added My organization is added to the README.
• I've signed the CLA and required builds are green.

Closes #1790

[simster7]

Implementation LGTM, although I think that my and @irlevesque's comments from #1791 should be considered: #1791 (comment) and #1791 (comment).

If you think it's fine regardless, then this should be good to merge.

[alexec]

@sarabala1979 @jessesuen myself and @irlevesque have implemented this feature separately, but slightly differently. The key difference is that he has introduced a new flag, whereas I have re-used an existing flag.

Pros/con of reuse are:

• Workflows with artifacts will already use this flag (side-effect).
• Anyone who has this flag will automatically start collecting logs for workflows that do not have artifacts.

My take is, that though people might not expect this change in behavior, that is to me what the intent of the configuration item is.

Thoughts.

[jessesuen]

Are you saying that if in the workflow-controller-configmap, archiveLogs: true is set, that we currently do not archive logs, but with this change it will? If so, I'm supportive of this change in behavior since that was the intent.

[Ark-kun]

Are you saying that if in the workflow-controller-configmap, archiveLogs: true is set, that we currently do not archive logs, but with this change it will? If so, I'm supportive of this change in behavior since that was the intent.

Yes, this is the case. I've just discovered this bug (in older Argo version) and I'm glad that it's already fixed. Thanks @alexec