-
Notifications
You must be signed in to change notification settings - Fork 1
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
Support ict (jobs and jobrunnergroup) #119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a few small cosmetic changes. LGTM mostly except for a couple of comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still reviewing. But blocking PR merge with request changes
to prevent merge of this PR with the hard coded bearer token.
add logs command add sse client lib
Canceling blocking review since the authorization change with hard-coded bearer token has been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went over the PR a little superficially and left some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple more comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran through some of the test cases and it looks good to start. Only pending comment I have is that we should have job get
output also reflect which jrg it's intending to run on. That's currently not visible. Rest of the behavior LGTM - @jmsktm - please approve once the code passes muster since you've been reviewing it more closely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more review comments added. Also tested everything other than artifact download which is being worked on, and is pending push.
internal/command/jobs/printers.go
Outdated
fmt.Fprintf(tw, "Status:\t%s\n", job.Status.Phase) | ||
fmt.Fprintf(tw, "Environment:\t%s\n", getJobEnvironment(job)) | ||
fmt.Fprintf(tw, "Created At:\t%s\n", getCreatedAt(job)) | ||
fmt.Fprintf(tw, "Started At:\t%s\n", createdAt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should display value corresponding to start time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed and tested locally. LGTM.
Support to full JobRunnerGroup and work for Jobs still going (pending to see logs, download artifacts, cancel job)