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

Support ict (jobs and jobrunnergroup) #119

Merged
merged 20 commits into from
May 30, 2024
Merged

Support ict (jobs and jobrunnergroup) #119

merged 20 commits into from
May 30, 2024

Conversation

davixcky
Copy link
Contributor

@davixcky davixcky commented May 24, 2024

Support to full JobRunnerGroup and work for Jobs still going (pending to see logs, download artifacts, cancel job)

@davixcky davixcky changed the title support ict Support ict (jobs and jobrunnergroup) May 24, 2024
@davixcky davixcky requested review from daniel-de-vera, scott-cotton and foxish and removed request for daniel-de-vera May 24, 2024 16:54
Copy link
Member

@foxish foxish left a 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

internal/command/jobs/printers.go Outdated Show resolved Hide resolved
internal/config/root.go Outdated Show resolved Hide resolved
@davixcky davixcky requested a review from foxish May 28, 2024 19:47
jmsktm
jmsktm previously requested changes May 28, 2024
Copy link
Contributor

@jmsktm jmsktm left a 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.

internal/command/artifact/command.go Outdated Show resolved Hide resolved
@signadot signadot deleted a comment from jmsktm May 28, 2024
@jmsktm jmsktm dismissed their stale review May 28, 2024 20:42

Canceling blocking review since the authorization change with hard-coded bearer token has been removed.

Copy link
Contributor

@jmsktm jmsktm left a 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.

internal/command/artifact/printers.go Outdated Show resolved Hide resolved
internal/command/artifact/printers.go Outdated Show resolved Hide resolved
internal/command/artifact/printers.go Outdated Show resolved Hide resolved
internal/command/artifact/printers.go Outdated Show resolved Hide resolved
internal/command/jobrunnergroup/apply.go Show resolved Hide resolved
internal/command/jobs/cancel.go Outdated Show resolved Hide resolved
internal/command/jobs/printers.go Outdated Show resolved Hide resolved
internal/command/logs/command.go Outdated Show resolved Hide resolved
internal/swagger/TextStream.go Outdated Show resolved Hide resolved
@davixcky
Copy link
Contributor Author

@jmsktm comments addressed, the only remains is the one that expect feedback from @foxish

Copy link
Contributor

@jmsktm jmsktm left a 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.

internal/command/jobrunnergroup/apply.go Outdated Show resolved Hide resolved
internal/command/jobrunnergroup/printers.go Show resolved Hide resolved
internal/command/jobs/printers.go Outdated Show resolved Hide resolved
internal/command/jobs/printers.go Outdated Show resolved Hide resolved
internal/command/jobs/printers.go Outdated Show resolved Hide resolved
internal/command/jobs/printers.go Show resolved Hide resolved
@davixcky davixcky requested a review from jmsktm May 29, 2024 23:41
Copy link
Member

@foxish foxish left a 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.

internal/command/artifact/get.go Outdated Show resolved Hide resolved
internal/command/artifact/get.go Outdated Show resolved Hide resolved
internal/command/jobs/printers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jmsktm jmsktm left a 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/config/root.go Outdated Show resolved Hide resolved
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)
Copy link
Contributor

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.

internal/config/job.go Outdated Show resolved Hide resolved
internal/command/jobs/printers.go Outdated Show resolved Hide resolved
internal/command/jobrunnergroup/command.go Show resolved Hide resolved
Copy link
Contributor

@jmsktm jmsktm left a 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.

@davixcky davixcky merged commit 9a91295 into main May 30, 2024
@davixcky davixcky deleted the support-ict branch May 30, 2024 17:54
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.

3 participants