feat: Allow for setting default configurations for workflows, Fixes #1923, #2044 #2331

NikeNano · 2020-02-28T15:59:25Z

This is a work in progress but would be happy for some initial feedback.

This is a bug fix for issue which is also discussed here. This PR aims to allow for having default values set for workflows. Which would allow for easier configurations of Arog workflows and also help for admin of jobs.

This is implemented after suggestions from @zhujl1991

Updated the naming and added comment

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>

codecov · 2020-02-28T16:16:00Z

Codecov Report

Merging #2331 into master will increase coverage by 0.01%.
The diff coverage is 31.81%.

@@            Coverage Diff            @@
##           master   #2331      +/-   ##
=========================================
+ Coverage   13.38%   13.4%   +0.01%     
=========================================
  Files          70      70              
  Lines       24232   24254      +22     
=========================================
+ Hits         3244    3251       +7     
- Misses      20579   20590      +11     
- Partials      409     413       +4

Impacted Files	Coverage Δ
workflow/config/config.go	`33.33% <ø> (ø)`	⬆️
workflow/controller/controller.go	`2.5% <31.81%> (+1.9%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efb8a1a...c5f5f2d. Read the comment docs.

alexec · 2020-02-28T22:50:04Z

workflow/config/config.go

+
+	// Workflow defaults
+	//[TODO] fix the representation
+	WorkflowDefaults *wfv1.WorkflowSpec `json:"workflowDefaults,omitempty"`


I think we should call this defautWorkflowSpec.

Should it be capital "d" to keep it consistent? DefautWorkflowSpec

workflow/controller/controller.go

NikeNano · 2020-02-29T07:15:14Z

Will uppdate based upon your comments!

workflow/controller/controller.go

workflow/controller/controller_test.go

NikeNano · 2020-03-04T12:51:25Z

Working on adding more test and fixing the current issues. Hope to have it done during the weekend.

alexec · 2020-03-04T17:26:02Z

Looks good. Did you want to add some docs for this? Maybe an example?

markterm · 2020-03-05T09:53:23Z

It could be quite useful for this to support default exit handlers

NikeNano · 2020-03-05T09:54:23Z

It could be quite useful for this to support default exit handlers

Where can I find more info/examples on this @mark9white ?

markterm · 2020-03-05T10:19:36Z

We have a generic handler like this that applies to all our workflows:

spec:
    onExit: exit-handler
  templates:
    ...
    - name: exit-handler
      steps:
        - - name: notify
            templateRef:
              name: slack-template
              template: send-slack-message
            arguments:
              parameters:
                - name: message
                  value: "Workflow {{workflow.name}} {{workflow.status}}"
                - name: channel
                  value: "dev-support-alerts"
            when: "{{workflow.status}} != Succeeded"

Though thinking about it an alternative solution would be to have a way to provide a templateRef to onExit and just onExit would need to be provided in the default workflow configuration.

NikeNano · 2020-03-05T12:33:53Z

@alexec I would be happy do to an example but I suggest to make that a separate PR. I am kind of happy with the test now. Would you like me to add some more test cases? Added some short documentation as well.

workflow/config/config.go

workflow/controller/controller.go

workflow/controller/controller_test.go

zhujl1991 · 2020-03-06T07:42:46Z

LGTM

NikeNano · 2020-03-06T07:50:27Z

support default exit handlers

@mark9white I dont really understand if this is suited here or not(because my lack of knowledge). Do you have an opinion about it @alexec ?

simster7

~~Very minor comments!~~ Two minutes too late... @NikeNano Could you open a separate PR and address these?

simster7 · 2020-03-06T15:57:11Z

workflow/config/config.go

@@ -64,6 +64,8 @@ type WorkflowControllerConfig struct {

 	// Config customized Docker Sock path
 	DockerSockPath string `json:"dockerSockPath,omitempty"`
+
+	DefautWorkflowSpec *wfv1.WorkflowSpec `json:"workflowDefaults,omitempty"`


Please add a docstring above this, like the rest of the configs

simster7 · 2020-03-06T15:57:45Z

workflow/controller/controller.go

+// workflowController. Values in the workflow will be given the upper hand over the defaults.
+// The defaults for the workflow controller is set in the WorkflowController.Config.DefautWorkflowSpec
+func (wfc *WorkflowController) addingWorkflowDefaultValueIfValueNotExist(wf *wfv1.Workflow) error {
+	//var workflowSpec *wfv1.WorkflowSpec = &wf.Spec


Remove this unused line?

simster7 · 2020-03-06T15:59:33Z

workflow/controller/controller.go

+		if err != nil {
+			return err
+		}
+		// https://github.com/kubernetes/apimachinery/blob/2373d029717c4d169463414a6127cd1d0d12680e/pkg/util/strategicpatch/patch.go#L94


Link to the docs directly? https://godoc.org/k8s.io/apimachinery/pkg/util/strategicpatch#StrategicMergePatch

…onfigurations #2331 (#2388)

NikeNano and others added 15 commits January 22, 2020 13:08

new start

2b9b943

updated to use commen object def

69276aa

updated the ttlcontroller to use the defaults if they are available

22756ff

test is running

9c17390

updated with more tests!

0baedc3

tests pass

822fba0

updated further to also check used default for expired

aca356b

updated with futher tests

50b27ec

missed pointer

79477ab

Merge branch 'master' into master

e034db7

Update config.go

18f45d6

Updated the naming and added comment

removed comment

75c6cc5

Update workflow/ttlcontroller/ttlcontroller_test.go

c6b91cc

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>

Merge branch 'master' into remote_master

81228ff

initial work

fcd03c1

alexec changed the title ~~Allow for setting default configurations for workflows, Fixes #1923, #2044[NOT DONE]~~ feat: Allow for setting default configurations for workflows, Fixes #1923, #2044[NOT DONE] Feb 28, 2020

alexec reviewed Feb 28, 2020

View reviewed changes

workflow/controller/controller.go Outdated Show resolved Hide resolved

alexec reviewed Feb 28, 2020

View reviewed changes

workflow/controller/controller.go Outdated Show resolved Hide resolved

NikeNano added 2 commits March 2, 2020 12:37

updated after feedback

1823b9e

updated the error handling

ab94227

golangcibot reviewed Mar 2, 2020

View reviewed changes

workflow/controller/controller.go Outdated Show resolved Hide resolved

workflow/controller/controller_test.go Outdated Show resolved Hide resolved

workflow/controller/controller_test.go Outdated Show resolved Hide resolved

NikeNano added 3 commits March 2, 2020 13:06

changed design in addingWorkflowdefault....

750e05f

updated

d4d1985

updated the tests

d7e8f55

alexec reviewed Mar 2, 2020

View reviewed changes

workflow/controller/controller_test.go Show resolved Hide resolved

need to update with further tests

f7bfb82

NikeNano added 2 commits March 4, 2020 15:09

misstake before we only want to do the merge

45ecb37

added more test cases

fdac75e

updated with short documentation

052d567

alexec requested changes Mar 5, 2020

View reviewed changes

workflow/config/config.go Outdated Show resolved Hide resolved

workflow/controller/controller.go Outdated Show resolved Hide resolved

workflow/controller/controller_test.go Show resolved Hide resolved

NikeNano added 2 commits March 5, 2020 19:00

Update config.go

c1bf852

Update controller.go

c5f5f2d

NikeNano changed the title ~~feat: Allow for setting default configurations for workflows, Fixes #1923, #2044[NOT DONE]~~ feat: Allow for setting default configurations for workflows, Fixes #1923, #2044 Mar 6, 2020

alexec approved these changes Mar 6, 2020

View reviewed changes

alexec merged commit 3890a12 into argoproj:master Mar 6, 2020

alexec added this to the v2.7 milestone Mar 6, 2020

simster7 suggested changes Mar 6, 2020

View reviewed changes

This was referenced Mar 8, 2020

fix(doc): Fix bug related documentation/clean up of default configurations #2331 #2387

Closed

fix(doc strings): Fix bug related documentation/clean up of default configurations #2331 #2388

Merged

alexec pushed a commit that referenced this pull request Mar 8, 2020

fix(doc strings): Fix bug related documentation/clean up of default c…

1d21d3f

…onfigurations #2331 (#2388)

This was referenced Mar 8, 2020

Provide a way to set default Workflow options that will apply to all workflows #1923

Closed

[Feature Request] Globally-defined logging to artifact repository #1790

Closed

ywskycn mentioned this pull request Mar 19, 2020

Add a config to allow user config main container resource #1248

Closed

KPostOffice mentioned this pull request Aug 24, 2020

Add TTLStrategy to all workflows thoth-station/thoth-application#302

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow for setting default configurations for workflows, Fixes #1923, #2044 #2331

feat: Allow for setting default configurations for workflows, Fixes #1923, #2044 #2331

NikeNano commented Feb 28, 2020

codecov bot commented Feb 28, 2020 •

edited

Loading

alexec Feb 28, 2020

NikeNano Mar 2, 2020

NikeNano commented Feb 29, 2020

NikeNano commented Mar 4, 2020

alexec commented Mar 4, 2020

markterm commented Mar 5, 2020

NikeNano commented Mar 5, 2020 •

edited

Loading

markterm commented Mar 5, 2020 •

edited

Loading

NikeNano commented Mar 5, 2020

zhujl1991 commented Mar 6, 2020

NikeNano commented Mar 6, 2020

simster7 left a comment •

edited

Loading

simster7 Mar 6, 2020

simster7 Mar 6, 2020

simster7 Mar 6, 2020

feat: Allow for setting default configurations for workflows, Fixes #1923, #2044 #2331

feat: Allow for setting default configurations for workflows, Fixes #1923, #2044 #2331

Conversation

NikeNano commented Feb 28, 2020

codecov bot commented Feb 28, 2020 • edited Loading

Codecov Report

alexec Feb 28, 2020

Choose a reason for hiding this comment

NikeNano Mar 2, 2020

Choose a reason for hiding this comment

NikeNano commented Feb 29, 2020

NikeNano commented Mar 4, 2020

alexec commented Mar 4, 2020

markterm commented Mar 5, 2020

NikeNano commented Mar 5, 2020 • edited Loading

markterm commented Mar 5, 2020 • edited Loading

NikeNano commented Mar 5, 2020

zhujl1991 commented Mar 6, 2020

NikeNano commented Mar 6, 2020

simster7 left a comment • edited Loading

Choose a reason for hiding this comment

simster7 Mar 6, 2020

Choose a reason for hiding this comment

simster7 Mar 6, 2020

Choose a reason for hiding this comment

simster7 Mar 6, 2020

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2020 •

edited

Loading

NikeNano commented Mar 5, 2020 •

edited

Loading

markterm commented Mar 5, 2020 •

edited

Loading

simster7 left a comment •

edited

Loading