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(tasks): CLI method to Find and Rerun failed Runs #20307

Merged
merged 31 commits into from
Feb 11, 2021
Merged

Conversation

Nav-aggarwal09
Copy link
Contributor

This PR aims to provide a CLI function to rerun failed runs.

If a specific taskID is given, it will rerun the runs with status "failed".
If no taskID is given, it will find all tasks and their associated "failed" runs and rerun them.

The proposal also begs the question whether this function can be added to the server side.

@Nav-aggarwal09
Copy link
Contributor Author

ignore test files for now. Need to implement builder pattern to make task.go testable.

@danxmoran
Copy link
Contributor

Drive-by comment: please make sure to add a line to CHANGELOG.md before merging

@Nav-aggarwal09
Copy link
Contributor Author

Note to self (and others reading): I was a bit slow making tests for the other subcommands, but wanted to push and show something passing.
As of now, the subcommands get all their env variables and flags from the builder and the first test has been written for task.go CLI which can then be referenced to make more tests for the subcommands.

@danxmoran danxmoran self-assigned this Jan 11, 2021
@danxmoran danxmoran self-requested a review January 11, 2021 19:18
@danxmoran danxmoran removed their assignment Jan 11, 2021
@Nav-aggarwal09 Nav-aggarwal09 changed the title CLI method to Find and Rerun failed Runs feat(tasks): CLI method to Find and Rerun failed Runs Jan 11, 2021
@danxmoran
Copy link
Contributor

danxmoran commented Jan 12, 2021

@Nav-aggarwal09 let me know when you think this is ready and I'll dig into the review

Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

Finishing review for now so I can see latest commits

cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Show resolved Hide resolved
cmd/influx/task.go Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

Some comments on the test suite

cmd/influx/task_test.go Outdated Show resolved Hide resolved
cmd/influx/task_test.go Show resolved Hide resolved
cmd/influx/task_test.go Show resolved Hide resolved
@Nav-aggarwal09
Copy link
Contributor Author

Nav-aggarwal09 commented Jan 20, 2021

@danxmoran Sorry I think I was cleaning up some last minute things while you were reviewing.

So there are 2 things I did:

  1. rerun-failed cmd implementation
  2. There was no way to test task.go because tests were not written, so I refactored it to how buckets was implemented -- builder pattern.

I didn't write all the tests since that's outside the scope of this PR (i think, since I was told it's a proposal), but it should provide the backbone and bare minimum for others to follow and build off of.

I will edit the code right now based on your review, and will ping you again soon.

@danxmoran danxmoran self-assigned this Feb 9, 2021
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

Thanks for your patience, I'll take a deeper look later today but noticed something easy

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@gavincabbage gavincabbage left a comment

Choose a reason for hiding this comment

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

Looking good. One more adjustment and I'm happy :)

cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Show resolved Hide resolved
cmd/influx/task.go Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

Talked with @gavincabbage on Slack, and decided that switching the name & default behavior of the CLI option is good 👍 left suggestions for the spots I saw that should change, but might have missed some spots

cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
cmd/influx/task.go Outdated Show resolved Hide resolved
Nav-aggarwal09 and others added 2 commits February 11, 2021 10:02
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

:shipit:

@danxmoran
Copy link
Contributor

Build failure is a known flaky thing, ok to ignore. Merging!

@danxmoran danxmoran merged commit afbe31d into master Feb 11, 2021
@Nav-aggarwal09
Copy link
Contributor Author

we made it 🙏

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.

5 participants