Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add suspend and resume to CronWorkflows CLI #1925

Merged
merged 5 commits into from
Jan 10, 2020

Conversation

simster7
Copy link
Member

@simster7 simster7 commented Jan 9, 2020

Closes: #1924

@codecov
Copy link

codecov bot commented Jan 9, 2020

Codecov Report

Merging #1925 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1925   +/-   ##
=======================================
  Coverage   10.65%   10.65%           
=======================================
  Files          35       35           
  Lines       24931    24931           
=======================================
  Hits         2657     2657           
  Misses      21935    21935           
  Partials      339      339

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 02022e4...a1aa9e5. Read the comment docs.

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests.

@simster7 simster7 requested a review from alexec January 9, 2020 18:26
@simster7
Copy link
Member Author

simster7 commented Jan 9, 2020

@alexec Added tests!

When().
CreateCronWorkflow().
Then().
RunCli([]string{"cron", "resume", "test-cron-wf-basic"}, func(t *testing.T, output string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... perhaps we can refactor the cli_test.go?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, however that is still only on the apiserverimpl branch. Maybe we can merge this, then I'll make the changes on that branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah - of course - we can merge from master into apiserverimpl

@@ -55,3 +59,17 @@ func (t *Then) ExpectWorkflowList(listOptions metav1.ListOptions, block func(*te
block(t.t, wfList)
return t
}

func (t *Then) RunCli(args []string, block func(*testing.T, string)) *Then {
cmd := exec.Command("../../../dist/argo", args...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please refactor the existing argo(...) command?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #1925 (comment)

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional approval - please run make lint as some imports looked like they were out of order.

@alexec
Copy link
Contributor

alexec commented Jan 10, 2020

Ignore e2e failure.

@simster7 simster7 merged commit 6af100d into argoproj:master Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to pause and resume CronWorkflows from the CLI
2 participants