Add AutoArchiveLogs option to send all workflow pod logs to Artifact Repository #1791
Conversation
|
|
| @@ -83,6 +83,8 @@ type KubeConfig struct { | |||
| type ArtifactRepository struct { | |||
| // ArchiveLogs enables log archiving | |||
| ArchiveLogs *bool `json:"archiveLogs,omitempty"` | |||
| // AutoArchiveLogs enables log archiving for all tasks regardless of workflow configuration | |||
| AutoArchiveLogs *bool `json:"autoArchiveLogs,omitempty"` | |||
There was a problem hiding this comment.
Maybe ForceArchiveLogs is more explicit?
There was a problem hiding this comment.
How about DefaultArchiveLogs? This change wouldn't force logs to be archived (users explicitly setting s3/artifactory destinations in template wouldn't be covered by this).
| @@ -834,6 +834,9 @@ func (woc *wfOperationCtx) addArchiveLocation(pod *apiv1.Pod, tmpl *wfv1.Templat | |||
| break | |||
| } | |||
| } | |||
| if woc.artifactRepository.AutoArchiveLogs != nil && *woc.artifactRepository.AutoArchiveLogs { | |||
| needLocation = true | |||
| } | |||
There was a problem hiding this comment.
Hmmm, currently it's not clear that even when enabling AutoArchiveLogs the user will also need to enable ArchiveLogs for the logs to be archived. Maybe a couple of lines below should actually be:
tmpl.ArchiveLocation = &wfv1.ArtifactLocation{
ArchiveLogs: woc.artifactRepository.ArchiveLogs || woc.artifactRepository.AutoArchiveLogs,
}Or something similar
There was a problem hiding this comment.
Agreed on clarity. I think your proposal sounds reasonable.
|
On more thought, I'm unsure if a config in What do you think? Can you think of another way to do this that would make the Workflow the active player in this? I want to know what @jessesuen thinks of this too |
|
I've just managed to re-implement this almost exactly the same in #1860 |
|
@irlevesque do you want to compare the two solutions, they're not exactly the same, as I re-use the |
|
Hi @alexec, my rationale for using a new variable was to avoid a change of behavior for existing users of the |
Fixes: #1790