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

close a verify-codegen ci failure loophole #5447

Merged
merged 1 commit into from Dec 9, 2021

Conversation

clnperez
Copy link
Contributor

@clnperez clnperez commented Dec 2, 2021

if the go codegen commands failed, the the hack/verify-codegen.sh script continued and the CI check passed. see #5406 for an example. the script was not set to fail on a non-zero return code. this pr adds the bash e option to the set builtin to catch errors and exit on failure.

Signed-off-by: Christy Norman christy@linux.vnet.ibm.com

@clnperez
Copy link
Contributor Author

clnperez commented Dec 2, 2021

The codegen tests fail now as they should:

Container test exited with code 1, reason Error
---
+ go generate ./pkg/types/installconfig.go
github.com/openshift/installer/pkg/types:-: conflicting types in allOf branches in schema: string vs DiskCategory
github.com/openshift/installer/pkg/types:-: conflicting types in allOf branches in schema: string vs DiskCategory
github.com/openshift/installer/pkg/types:-: conflicting types in allOf branches in schema: string vs DiskCategory
Error: not all generators ran successfully 

We should merge #5406 first

@clnperez
Copy link
Contributor Author

clnperez commented Dec 2, 2021

fyi @patrickdillon

@jhixson74
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2021
@squidboylan
Copy link
Contributor

looks good to me

@@ -3,7 +3,15 @@
if [ "$IS_CONTAINER" != "" ]; then
set -x
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove the set -x and set +x instead of going through the machinations of checking the return codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i added e instead

rc=$?
if [ $rc != 0 ]; then
exit 1
fi
go generate ./pkg/rhcos/ami.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to fail because the file no longer exists. We need a replacement for this. But we can comment it out for now.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2021
@clnperez clnperez force-pushed the ci-codegen branch 2 times, most recently from 1786131 to 6f6b93a Compare December 6, 2021 22:14
@clnperez
Copy link
Contributor Author

clnperez commented Dec 7, 2021

@staebler @jhixson74 PTAL at the updates

Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

The changes look good.
Please improve upon the PR and commit title so that they are not quite so generic.

@staebler
Copy link
Contributor

staebler commented Dec 7, 2021

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 7, 2021
@clnperez clnperez changed the title close a codegen loophole close a verify-codegen ci failure loophole Dec 7, 2021
@clnperez
Copy link
Contributor Author

clnperez commented Dec 7, 2021

i updated the summary comment and pr title. hopefully gh picks those up.

@staebler
Copy link
Contributor

staebler commented Dec 7, 2021

i updated the summary comment and pr title. hopefully gh picks those up.

You need to push up the commit with the updated message.

@clnperez
Copy link
Contributor Author

clnperez commented Dec 8, 2021

@staebler done

git diff --exit-code
set +ex
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave this up above the git diff as it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops. yup.

if the go codegen commands failed, the the hack/verify-codegen.sh script
continued and the CI check passed. see openshift#5406 for an example. the script
was not set to fail on a non-zero return code. this pr adds the bash e
option to the set builtin to catch errors and exit on failure.

Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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

Comment on lines +6 to +7
# See https://github.com/openshift/installer/pull/5447#discussion_r762340594
# go generate ./pkg/rhcos/ami.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed with #5466.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds like i should just delete those two commented-out lines now then @staebler?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, leave them commented out. This PR will likely merge before mine.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

6 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 9, 2021

@clnperez: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-libvirt 421e1e2 link false /test e2e-libvirt
ci/prow/e2e-crc 421e1e2 link false /test e2e-crc
ci/prow/e2e-aws-fips 421e1e2 link false /test e2e-aws-fips
ci/prow/okd-e2e-aws 421e1e2 link false /test okd-e2e-aws
ci/prow/e2e-openstack 421e1e2 link false /test e2e-openstack
ci/prow/e2e-ovirt 421e1e2 link false /test e2e-ovirt
ci/prow/e2e-metal-ipi-ovn-ipv6 421e1e2 link false /test e2e-metal-ipi-ovn-ipv6

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

15 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit eaaf64f into openshift:master Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants