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.

Sign up for GitHub

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

Jump to bottom

fix(cli): Validate cron on update. Fixes #5691 #5692

Merged
merged 2 commits into from
Apr 19, 2021
Merged

fix(cli): Validate cron on update. Fixes #5691 #5692

merged 2 commits into from
Apr 19, 2021

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Apr 17, 2021

No description provided.

@alexec alexec linked an issue Apr 17, 2021 that may be closed by this pull request
CronWorkflow schedule giving inconsistent experience #5691
Closed
@alexec alexec added this to the v3.0 milestone Apr 17, 2021
@codecov
Copy link

codecov bot commented Apr 17, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@91c08cd). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5692   +/-   ##
=========================================
  Coverage          ?   46.96%           
=========================================
  Files             ?      244           
  Lines             ?    15214           
  Branches          ?        0           
=========================================
  Hits              ?     7145           
  Misses            ?     7164           
  Partials          ?      905
Impacted Files Coverage Δ
server/cronworkflow/cron_workflow_server.go 65.07% <100.00%> (ø)

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 91c08cd...50bc783. Read the comment docs.

@alexec alexec marked this pull request as draft April 17, 2021 22:03
@alexec
ok
50bc783 
Signed-off-by: Alex Collins <[email protected]>
@alexec alexec marked this pull request as ready for review April 18, 2021 01:09
terrytangyuan
terrytangyuan reviewed Apr 18, 2021
View reviewed changes
ui/src/app/cron-workflows/components/schedule-validator.tsx
@@ -5,6 +5,9 @@ const x = require('cronstrue');

export const ScheduleValidator = ({schedule}: {schedule: string}) => {
try {
if (schedule.split(' ').length === 6) {
Copy link
Member

@terrytangyuan terrytangyuan Apr 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also provide an error message when length > 6 so users can be noted of mistakes earlier in the UI?

@alexec alexec merged commit b267e3c into argoproj:master Apr 19, 2021
@alexec alexec deleted the fix-cron branch April 19, 2021 15:45
@simster7 simster7 mentioned this pull request Apr 19, 2021
Closed
50 tasks
simster7 pushed a commit that referenced this pull request Apr 19, 2021
@alexec @simster7
fix(cli): Validate cron on update. Fixes #5691 (#5692)
279b78b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrytangyuan terrytangyuan terrytangyuan left review comments

@sarabala1979 sarabala1979 sarabala1979 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0
Development

Successfully merging this pull request may close these issues.

CronWorkflow schedule giving inconsistent experience
3 participants
@alexec @terrytangyuan @sarabala1979