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

quadlet should exit non zero on failures #18828

Merged
merged 1 commit into from
Jun 18, 2023
Merged

quadlet should exit non zero on failures #18828

merged 1 commit into from
Jun 18, 2023
+172 −136

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jun 8, 2023 •
edited
Loading

Does this PR introduce a user-facing change?

Fixes: #18778

quadlet will now exit non-zero when errors found.

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 8, 2023
rhatdan
rhatdan commented Jun 8, 2023
View reviewed changes
test/e2e/quadlet_test.go Outdated Show resolved Hide resolved
@rhatdan
Copy link
Member Author

rhatdan commented Jun 8, 2023

@alexlarsson @vrothberg @ygalblum PTAL

@rhatdan rhatdan force-pushed the quadlet branch 2 times, most recently from c617d6c to 2115af1 Compare June 9, 2023 16:48
ygalblum
ygalblum reviewed Jun 11, 2023
View reviewed changes
cmd/quadlet/main.go Outdated Show resolved Hide resolved
cmd/quadlet/main.go Outdated Show resolved Hide resolved
test/e2e/quadlet_test.go
@@ -511,7 +526,7 @@ var _ = Describe("quadlet system generator", func() {
})

DescribeTable("Running quadlet test case",
func(fileName string) {
func(fileName string, exitCode int, errString string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will be cleaner to move the actual implementation of this function to an external function and then split between the test cases into two tables. The first will be remain as before and func will pass hardcoded 0 "", while the second will require the exit code and string from the entries.

vrothberg
vrothberg reviewed Jun 12, 2023
View reviewed changes
cmd/quadlet/main.go Outdated Show resolved Hide resolved
cmd/quadlet/main.go Outdated Show resolved Hide resolved
@vrothberg vrothberg mentioned this pull request Jun 12, 2023
Closed
@vrothberg
Copy link
Member

@rhatdan on repush, can you add a Fixes: #18778?

@rhatdan rhatdan force-pushed the quadlet branch 4 times, most recently from 786b89e to 27fe927 Compare June 13, 2023 17:51
vrothberg
vrothberg reviewed Jun 14, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Just some final nits on error handling in main().

cmd/quadlet/main.go Outdated Show resolved Hide resolved
cmd/quadlet/main.go Outdated Show resolved Hide resolved
cmd/quadlet/main.go Outdated Show resolved Hide resolved
vrothberg
vrothberg approved these changes Jun 14, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vrothberg
Copy link
Member

@ygalblum PTanotherL

@ygalblum
Copy link
Contributor

LGTM for the code but the tests are unhappy

@rhatdan rhatdan force-pushed the quadlet branch 2 times, most recently from f37ec00 to 8c37e62 Compare June 15, 2023 02:56
@ygalblum
Copy link
Contributor

@rhatdan something broke all the Quadlet e2e tests

@rhatdan rhatdan force-pushed the quadlet branch 2 times, most recently from 3391d16 to acb9ebb Compare June 15, 2023 16:38
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2023
@ygalblum
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2023
@openshift-merge-robot openshift-merge-robot merged commit feea666 into containers:main Jun 18, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 17, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@ygalblum ygalblum ygalblum left review comments

@edsantiago edsantiago edsantiago left review comments

@vrothberg vrothberg vrothberg approved these changes

Assignees

@ygalblum ygalblum

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Quadlet for adhoc units
6 participants
@rhatdan @vrothberg @ygalblum @edsantiago @Luap99 @openshift-merge-robot