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(ci): add helpful error when check-generated-code gha workflow fails #627

Merged
merged 6 commits into from
Jul 19, 2024

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented Jul 19, 2024

Description

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@jiridanek jiridanek requested a review from atheo89 July 19, 2024 08:43
@@ -23,6 +23,7 @@ jobs:
if [[ -z "$clean" ]]; then
echo "Empty git status --porcelain: $clean"
else
echo "::error::Please run the command from the previous step (Rerun all code generators...) and commit the result."
Copy link
Member

Choose a reason for hiding this comment

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

Please, instead of telling to run command from previous step, maybe we should explicitly say what to run.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, then, let me change this.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -15,14 +15,15 @@ jobs:
- uses: actions/checkout@v4

- name: Rerun all code generators we have
run: python3 ci/cached-builds/gen_gha_matrix_jobs.py
run: bash ci/generate_code.bash
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the .bash suffix :P , I can see we have it in one file already ./ci/cached-builds/gha_lvm_overlay.bash... still, do we want to extend this policy?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's only tested with bash; when I get the mac, I will also test it on zsh

Copy link
Member

Choose a reason for hiding this comment

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

What does the file suffix have to do with the which interpreter is used?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed back to .sh, for consistency


if ! python3 ci/cached-builds/gen_gha_matrix_jobs.py; then
# this returns nonzero when running outside of github; detect and print explanation
if [[ ${GITHUB_ACTIONS:-} == true ]]; then exit 1
Copy link
Member

Choose a reason for hiding this comment

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

It's a pitty that the script fails when non-run in github... I consider this solution as a workaround that will be fixed and won't be needed in the near future; we may miss other reasons why the script failed when run locally with this approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's not a good job on my part.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was not too had to fix, did it rn

@atheo89
Copy link
Member

atheo89 commented Jul 19, 2024

/lgtm

@openshift-ci openshift-ci bot removed the lgtm label Jul 19, 2024
@jstourac
Copy link
Member

Thank you, I like this much more now!

/lgtm

@openshift-ci openshift-ci bot removed the lgtm label Jul 19, 2024
Copy link
Contributor

openshift-ci bot commented Jul 19, 2024

New changes are detected. LGTM label has been removed.

Copy link
Contributor

openshift-ci bot commented Jul 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

@jiridanek jiridanek added the lgtm label Jul 19, 2024
@openshift-ci openshift-ci bot removed the lgtm label Jul 19, 2024
Copy link
Contributor

openshift-ci bot commented Jul 19, 2024

New changes are detected. LGTM label has been removed.

@jiridanek jiridanek added the lgtm label Jul 19, 2024
@jiridanek
Copy link
Member Author

/override ci/prow/runtime-rocm-pytorch-ubi9-python-3-9-pr-image-mirror
/override ci/prow/amd-runtimes-ubi9-e2e-tests
/override ci/prow/images

one rocm run out of ephemeral storage, and tests hit the chicken-and-egg problem from openshift/release#54567

Copy link
Contributor

openshift-ci bot commented Jul 19, 2024

@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/amd-runtimes-ubi9-e2e-tests, ci/prow/images, ci/prow/runtime-rocm-pytorch-ubi9-python-3-9-pr-image-mirror

In response to this:

/override ci/prow/runtime-rocm-pytorch-ubi9-python-3-9-pr-image-mirror
/override ci/prow/amd-runtimes-ubi9-e2e-tests
/override ci/prow/images

one rocm run out of ephemeral storage, and tests hit the chicken-and-egg problem from openshift/release#54567

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-sigs/prow repository.

@openshift-merge-bot openshift-merge-bot bot merged commit a1154b4 into opendatahub-io:main Jul 19, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants