feat(cli): Add cost optimization nudges for Argo CLI #5168

dinever · 2021-02-23T05:59:52Z

Hi guys, this one closes #4548

Demo:

This is my first contribution to Argo. Would❤️ to hear some feedback. Thanks!

Checklist:

My organization is added to USERS.md.

simster7

Great! Thanks for submitting this. This looks great so far. Two things:

Users should be able to turn off nudges. I now see that our CLI does not store any state anywhere. Not sure that we would want to add a CLI config/data store (e.g. in ~/.argo/config) just for this, but that is a possibility. Do you have any other ideas for this?

We have another nudge in our UI:

argo-workflows/ui/src/app/workflows/components/workflow-details/workflow-details.tsx

Lines 121 to 123 in 4b78a7e

    
           if (!execSpec(workflow).securityContext) { 
        
               return <SecurityNudge>This workflow does not have security context set. It maybe possible to set this to run it more securely.</SecurityNudge>; 
        
           }

Could we add that as well?

simster7 · 2021-02-23T16:16:36Z

util/printer/workflow-printer.go

+	completed, incomplete := countCompletedWorkflows(wfList)
+	if completed > 100 || incomplete > 100 {
+		_, _ = fmt.Fprint(w, "\n")
+		fmt.Fprintf(w, "You have at least ")


Should this also disregard returned values for consistency?

Suggested change

fmt.Fprintf(w, "You have at least ")

_, _ = fmt.Fprintf(w, "You have at least ")

dinever · 2021-02-23T22:03:14Z

Thanks for the review @simster7!

Not sure that we would want to add a CLI config/data store (e.g. in ~/.argo/config) just for this, but that is a possibility.

I'm happy to implement this. However, a config data store feels like an overkill for this nudges use case unless we have other use cases that can benefit from it as well.

Another option is that we can add a --disable-nudges option for the CLI so that users can choose to disable it.

Let me know what you think. Thanks!

Signed-off-by: Peixuan Ding <dingpeixuan911@gmail.com>

dinever · 2021-02-24T06:16:58Z

Hi @simster7,

Thanks for the review! I made some changes based on your feedback.

af3f1f6: Added a --no-nudges option for the CLI impacted. User can choose to not print the nudges
ffe487f: Added a security nudge for argo get
c610490: Discard return values for fmt.Print and fmt.Printf for consistency

Let me know your thoughts. Thanks!

Signed-off-by: Peixuan Ding <dingpeixuan911@gmail.com>

alexec · 2021-02-24T16:24:15Z

My thoughts on this:

Great stuff!
I'm not sure it is worth it to make it possible to disable nudges in the CLI. Instead, let's see if people ask for that feature.
Should we do security nudges too?
Nudges need to use "exec spec". I think there is Workflow.ExecSpec() for that.

simster7

This LGTM, I'd like @alexec to also take a look since he wrote the initial nudges

simster7 · 2021-02-24T16:32:05Z

Should we do security nudges too?

I think security nudges are already implemented.

Nudges need to use "exec spec". I think there is Workflow.ExecSpec() for that.

Exec spec is only generated and kept in the controller... this is all CLI code

alexec · 2021-02-24T16:36:41Z

util/printer/workflow-printer.go

+			_, _ = fmt.Fprintf(w, "%d completed ", completed)
+		}
+		_, _ = fmt.Fprint(w, "workflows. Reducing the total number of workflows will reduce your costs.\n")
+		_, _ = fmt.Fprint(w, "Learn more at https://argoproj.github.io/argo-workflows/cost-optimisation/\n")


can we move the security nudge code here please?

alexec · 2021-02-24T16:37:19Z

util/printer/workflow-printer.go

+		if completed > 100 {
+			_, _ = fmt.Fprintf(w, "%d completed ", completed)
+		}
+		_, _ = fmt.Fprint(w, "workflows. Reducing the total number of workflows will reduce your costs.\n")


minor - could you use Fprintln?

… pkg Signed-off-by: Peixuan Ding <dingpeixuan911@gmail.com>

dinever force-pushed the cost-optitmiztion-cli branch 3 times, most recently from 5e20696 to 50bb8af Compare February 23, 2021 06:47

simster7 reviewed Feb 23, 2021

View reviewed changes

simster7 self-assigned this Feb 23, 2021

dinever force-pushed the cost-optitmiztion-cli branch from fa3e497 to 7a67690 Compare February 24, 2021 06:07

dinever added 4 commits February 24, 2021 01:08

feat(cli): Add cost optimization nudges for Argo CLI

47be731

Signed-off-by: Peixuan Ding <dingpeixuan911@gmail.com>

feat(cli): Add --no-nudges option to disable nudges output for argo list

af3f1f6

Signed-off-by: Peixuan Ding <dingpeixuan911@gmail.com>

feat(cli): Add security nudges for get command

ffe487f

Signed-off-by: Peixuan Ding <dingpeixuan911@gmail.com>

feat(cli): Discard returned values for consistency

c610490

Signed-off-by: Peixuan Ding <dingpeixuan911@gmail.com>

dinever force-pushed the cost-optitmiztion-cli branch from 7a67690 to c610490 Compare February 24, 2021 06:08

feat(cli): Update docs

6cfa596

Signed-off-by: Peixuan Ding <dingpeixuan911@gmail.com>

dinever force-pushed the cost-optitmiztion-cli branch from 510a373 to 6cfa596 Compare February 24, 2021 06:25

feat(cli): goimports

0827ccc

Signed-off-by: Peixuan Ding <dingpeixuan911@gmail.com>

dinever force-pushed the cost-optitmiztion-cli branch from 519b8bd to 0827ccc Compare February 24, 2021 07:06

simster7 approved these changes Feb 24, 2021

View reviewed changes

alexec requested changes Feb 24, 2021

View reviewed changes

refactor(cli): Move security nudges printing code to the printer util…

983c51e

… pkg Signed-off-by: Peixuan Ding <dingpeixuan911@gmail.com>

dinever force-pushed the cost-optitmiztion-cli branch from fa8c644 to 983c51e Compare February 25, 2021 05:12

alexec approved these changes Feb 25, 2021

View reviewed changes

alexec merged commit 4881111 into argoproj:master Feb 25, 2021

This was referenced Mar 1, 2021

v3.0.0-rc4 cherry-pick #5247

Closed

v2.12.10 cherry-pick #5326

Closed

dinever deleted the cost-optitmiztion-cli branch April 15, 2021 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): Add cost optimization nudges for Argo CLI #5168

feat(cli): Add cost optimization nudges for Argo CLI #5168

dinever commented Feb 23, 2021 •

edited

Loading

simster7 left a comment

simster7 Feb 23, 2021

dinever commented Feb 23, 2021

dinever commented Feb 24, 2021

alexec commented Feb 24, 2021

simster7 left a comment

simster7 commented Feb 24, 2021 •

edited

Loading

alexec Feb 24, 2021

alexec Feb 24, 2021

	if (!execSpec(workflow).securityContext) {
	return <SecurityNudge>This workflow does not have security context set. It maybe possible to set this to run it more securely.</SecurityNudge>;
	}

	fmt.Fprintf(w, "You have at least ")
	_, _ = fmt.Fprintf(w, "You have at least ")

feat(cli): Add cost optimization nudges for Argo CLI #5168

feat(cli): Add cost optimization nudges for Argo CLI #5168

Conversation

dinever commented Feb 23, 2021 • edited Loading

simster7 left a comment

Choose a reason for hiding this comment

simster7 Feb 23, 2021

Choose a reason for hiding this comment

dinever commented Feb 23, 2021

dinever commented Feb 24, 2021

alexec commented Feb 24, 2021

simster7 left a comment

Choose a reason for hiding this comment

simster7 commented Feb 24, 2021 • edited Loading

alexec Feb 24, 2021

Choose a reason for hiding this comment

alexec Feb 24, 2021

Choose a reason for hiding this comment

dinever commented Feb 23, 2021 •

edited

Loading

simster7 commented Feb 24, 2021 •

edited

Loading