feat(gh-action): ensures generated files are up-to-date in PR #917

AjayJagan · 2024-03-13T07:02:34Z

This is a follow up task related to https://issues.redhat.com/browse/RHOAIENG-4185

Fixes: https://issues.redhat.com/browse/RHOAIENG-4433

Description

In the ODH repository, whenever a change is made in the apis/ folder, it updates the CRDS and README(of the API). Many times devs forget to run this command and as a result, there are some mismatches in these files.

The idea is to create a gh action that will check if these updates are made properly and will prevent the merge of the PR if such a situation arises.

ref: #890 (comment)

Additionally, we may need to use Require status checks to pass before merging setting in GH to prevent the merge of the pr if the check does not pass.

How Has This Been Tested?

To test this change

pull the changes
make changes to the apis/ folder and don't run make generate or make manifests or make api-docs
Create a PR to your incubation branch and see that the PR fails with a comment

- If the above-said commands are run and the pr is updated, then the check will pass.

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

AjayJagan · 2024-03-13T08:23:10Z

/retest-required

AjayJagan · 2024-03-13T11:34:21Z

/retest-required

.github/workflows/check-file-updates.yaml

bartoszmajsak · 2024-03-13T12:28:19Z

I find it inconvenient that for such PRs we have to trigger operator-specific jobs (image creation, tests). These are unrelated changes. By doing so "time to merge" takes longer for no good reason. Not to mention it has a cost.

I think for everything .github/*, some other configs (like .gitignore) and perhaps *.md files we could skip ci/prow jobs. I think this could help in achieving this - https://docs.prow.k8s.io/docs/jobs/#triggering-jobs-based-on-changes

WDYT?

AjayJagan · 2024-03-13T12:31:26Z

I find it inconvenient that for such PRs we have to trigger operator-specific jobs (image creation, tests). These are unrelated changes. By doing so "time to merge" takes longer for no good reason. Not to mention it has a cost.

I think for everything .github/*, some other configs (like .gitignore) and perhaps *.md files we could skip ci/prow jobs. I think this could help in achieving this - https://docs.prow.k8s.io/docs/jobs/#triggering-jobs-based-on-changes

WDYT?

so what you mean is for changes related to .gitignore, md files etc you want the skip the ci/prow jobs right?

bartoszmajsak · 2024-03-13T13:12:33Z

I find it inconvenient that for such PRs we have to trigger operator-specific jobs (image creation, tests). These are unrelated changes. By doing so "time to merge" takes longer for no good reason. Not to mention it has a cost.
I think for everything .github/*, some other configs (like .gitignore) and perhaps *.md files we could skip ci/prow jobs. I think this could help in achieving this - docs.prow.k8s.io/docs/jobs/#triggering-jobs-based-on-changes
WDYT?

so what you mean is for changes related to .gitignore, md files etc you want the skip the ci/prow jobs right?

AjayJagan · 2024-03-14T09:08:13Z

I find it inconvenient that for such PRs we have to trigger operator-specific jobs (image creation, tests). These are unrelated changes. By doing so "time to merge" takes longer for no good reason. Not to mention it has a cost.

I think for everything .github/*, some other configs (like .gitignore) and perhaps *.md files we could skip ci/prow jobs. I think this could help in achieving this - https://docs.prow.k8s.io/docs/jobs/#triggering-jobs-based-on-changes

WDYT?

@bartoszmajsak @ykaliuta @zdtsw : created a pr to prevent the unwanted running of prow jobs: openshift/release#49860
This is my understanding, let me know if it needs any changes :)

bartoszmajsak

That's a really helpful addition! Thanks @AjayJagan.

I have a few suggestions, mostly about naming. From the functionality perspective it's solid.

.github/workflows/check-file-updates.yaml

bartoszmajsak

Sorry for ping-pong review... I missed one thing here.

.github/workflows/check-file-updates.yaml

bartoszmajsak

LGTM

bartoszmajsak · 2024-03-14T16:42:04Z

.github/workflows/check-file-updates.yaml

+          CMD="make generate manifests api-docs"
+          $CMD
+          echo "CMD=$CMD" >> $GITHUB_OUTPUT


This is how I originally started, but wanted to be more fancy with fc :) that's fine I guess, though shellcheck will freak out :)

openshift-ci · 2024-03-14T19:15:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bartoszmajsak, zdtsw

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:

~~OWNERS~~ [zdtsw]

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

aslakknutsen · 2024-03-14T22:25:59Z

A PR Comment is nice, A Commit pushed to the branch with the updated changes; priceless.. ;)

AjayJagan · 2024-03-15T04:27:28Z

A PR Comment is nice, A Commit pushed to the branch with the updated changes; priceless.. ;)

@aslakknutsen , I wanted that power delegated to the pr creator itself 😅, that is why I chose this path.

zdtsw · 2024-03-15T12:00:11Z

i just realized a funny thing:
the pre-built binary of crd-ref-docs is only for x86_64, it wont work on M1 M2

AjayJagan · 2024-03-15T12:10:44Z

that is true, maybe we can create an issue in their repo asking to release an arm-supported version.

zdtsw · 2024-03-15T12:37:02Z

that is true, maybe we can create an issue in their repo asking to release an arm-supported version.

i can build it myself, but @VaishnaviHire will bump this issue next week

…t-data-services#917) * RHOAIENG-4433 Update readme/config files * rework gh action based on comments * feat(gh-action): ensures generated files are up-to-date in PR * fix review comments * reuse cmd

* feat(gh-action): ensures generated files are up-to-date in PR (#917) * RHOAIENG-4433 Update readme/config files * rework gh action based on comments * feat(gh-action): ensures generated files are up-to-date in PR * fix review comments * reuse cmd * fix(gen): upgrades to crd-ref-doc generator to latest version (#920) fix(gen): upgrades to crd-ref-doc to latest version - add "diff" in the failed GH action to help understand problem - update: api doc --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> * fix: Enable annotations using golangci-lint(gh action) (#919) * fix: Enable annotations using golangci-lint(gh action) * update versions * fix(make): cleans linter cache on clean (#922) When juggling between branches golangci linter cache can have leftovers from the code that does not exist in the other branch. This can lead to errors while running `make lint` as the linter tries to check cached sources first. This change cleans the linter cache as part of the clean target to ensure consistency. Here's an example of an invalid cache state leading to error: ``` WARN [runner] Can't process result by autogenerated_exclude processor: can't filter issue result.Issue{FromLinter:"ireturn", Text:"Entry returns interface (github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature.DataProvider)", Severity:"", SourceLines:[]string(nil), Replacement:(*result.Replacement)(nil), Pkg:(*packages.Package)(0xc001343e00), LineRange:(*result.Range)(nil), Pos:token.Position{Filename:"/home/bartek/code/rhods/opendatahub-operator/incubation/pkg/feature/data_provider.go", Offset:1382, Line:45, Column:1}, HunkPos:0, ExpectNoLint:false, ExpectedNoLintLinter:""}: failed to get doc of file ``` * chore(Github): update issue template re-direct user for jira (#838) * chore(Github): update issue template re-direct user for jira Signed-off-by: Wen Zhou <wenzhou@redhat.com> * Update .github/ISSUE_TEMPLATE/feature_request.md Co-authored-by: Ajay Jaganathan <36824134+AjayJagan@users.noreply.github.com> * Update .github/ISSUE_TEMPLATE/bug_report.md Co-authored-by: Ajay Jaganathan <36824134+AjayJagan@users.noreply.github.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> Co-authored-by: Ajay Jaganathan <36824134+AjayJagan@users.noreply.github.com> * chore: Small Makefile fixes (#928) * Go back to using `IMG ?=` rather than forcing the value with `IMG =`. This makes it easier to override the value of `IMG` for development, and it also aligns it to how `BUNDLE_IMG` is assigned. * Add `prepare` as dependency of the `undeploy` target. This is just for convenience, although when looking at the other targets, `prepare` may simply be a missing dependency in `undeploy`. Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> * fix(test): pin version not latest which just introduced to use go 1.22 (#934) * fix(test): pin version not latest which just introduced to use go 1.22 Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: code review Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com> Co-authored-by: Ajay Jaganathan <36824134+AjayJagan@users.noreply.github.com> Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com> Co-authored-by: Edgar Hernández <ehernand@redhat.com>

openshift-ci bot requested review from lucferbux and VedantMahabaleshwarkar March 13, 2024 07:02

AjayJagan requested review from bartoszmajsak, VaishnaviHire and zdtsw and removed request for lucferbux and VedantMahabaleshwarkar March 13, 2024 07:02

AjayJagan force-pushed the add-gh-action-for-make-commands branch from c0b093b to 09144d2 Compare March 13, 2024 07:04

AjayJagan requested a review from aslakknutsen March 13, 2024 07:06

RHOAIENG-4433 Update readme/config files

ad6d755

AjayJagan force-pushed the add-gh-action-for-make-commands branch from 09144d2 to ad6d755 Compare March 13, 2024 07:10

zdtsw reviewed Mar 13, 2024

View reviewed changes

.github/workflows/check-file-updates.yaml Outdated Show resolved Hide resolved

.github/workflows/check-file-updates.yaml Outdated Show resolved Hide resolved

.github/workflows/check-file-updates.yaml Outdated Show resolved Hide resolved

rework gh action based on comments

527ee06

bartoszmajsak requested changes Mar 14, 2024

View reviewed changes

openshift-ci bot assigned bartoszmajsak Mar 14, 2024

bartoszmajsak reviewed Mar 14, 2024

View reviewed changes

.github/workflows/check-file-updates.yaml Outdated Show resolved Hide resolved

bartoszmajsak reviewed Mar 14, 2024

View reviewed changes

.github/workflows/check-file-updates.yaml Outdated Show resolved Hide resolved

AjayJagan force-pushed the add-gh-action-for-make-commands branch from 37713a3 to a41f3a2 Compare March 14, 2024 09:36

AjayJagan requested a review from bartoszmajsak March 14, 2024 09:45

feat(gh-action): ensures generated files are up-to-date in PR

7e9b6ba

AjayJagan force-pushed the add-gh-action-for-make-commands branch from a41f3a2 to 7e9b6ba Compare March 14, 2024 11:37

AjayJagan changed the title ~~RHOAIENG-4433 Update readme/config files~~ feat(gh-action): ensures generated files are up-to-date in PR Mar 14, 2024

bartoszmajsak requested changes Mar 14, 2024

View reviewed changes

.github/workflows/check-file-updates.yaml Outdated Show resolved Hide resolved

.github/workflows/check-file-updates.yaml Outdated Show resolved Hide resolved

.github/workflows/check-file-updates.yaml Outdated Show resolved Hide resolved

zdtsw reviewed Mar 14, 2024

View reviewed changes

.github/workflows/check-file-updates.yaml Show resolved Hide resolved

.github/workflows/check-file-updates.yaml Outdated Show resolved Hide resolved

.github/workflows/check-file-updates.yaml Show resolved Hide resolved

AjayJagan added 2 commits March 14, 2024 20:36

fix review comments

b26589f

reuse cmd

37bd52a

bartoszmajsak approved these changes Mar 14, 2024

View reviewed changes

openshift-ci bot added the lgtm label Mar 14, 2024

zdtsw approved these changes Mar 14, 2024

View reviewed changes

openshift-ci bot assigned zdtsw Mar 14, 2024

openshift-ci bot added the approved label Mar 14, 2024

zdtsw merged commit 3eae1ea into opendatahub-io:incubation Mar 14, 2024
7 of 8 checks passed

bartoszmajsak mentioned this pull request Mar 25, 2024

dead code #936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gh-action): ensures generated files are up-to-date in PR #917

feat(gh-action): ensures generated files are up-to-date in PR #917

AjayJagan commented Mar 13, 2024 •

edited

Loading

AjayJagan commented Mar 13, 2024

AjayJagan commented Mar 13, 2024

bartoszmajsak commented Mar 13, 2024

AjayJagan commented Mar 13, 2024

bartoszmajsak commented Mar 13, 2024

AjayJagan commented Mar 14, 2024

bartoszmajsak left a comment

bartoszmajsak left a comment

bartoszmajsak left a comment

bartoszmajsak Mar 14, 2024

openshift-ci bot commented Mar 14, 2024

aslakknutsen commented Mar 14, 2024

AjayJagan commented Mar 15, 2024

zdtsw commented Mar 15, 2024

AjayJagan commented Mar 15, 2024

zdtsw commented Mar 15, 2024

feat(gh-action): ensures generated files are up-to-date in PR #917

feat(gh-action): ensures generated files are up-to-date in PR #917

Conversation

AjayJagan commented Mar 13, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

AjayJagan commented Mar 13, 2024

AjayJagan commented Mar 13, 2024

bartoszmajsak commented Mar 13, 2024

AjayJagan commented Mar 13, 2024

bartoszmajsak commented Mar 13, 2024

AjayJagan commented Mar 14, 2024

bartoszmajsak left a comment

Choose a reason for hiding this comment

bartoszmajsak left a comment

Choose a reason for hiding this comment

bartoszmajsak left a comment

Choose a reason for hiding this comment

bartoszmajsak Mar 14, 2024

Choose a reason for hiding this comment

openshift-ci bot commented Mar 14, 2024

aslakknutsen commented Mar 14, 2024

AjayJagan commented Mar 15, 2024

zdtsw commented Mar 15, 2024

AjayJagan commented Mar 15, 2024

zdtsw commented Mar 15, 2024

AjayJagan commented Mar 13, 2024 •

edited

Loading