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

ci: update action to check git diff #59

Merged
merged 2 commits into from
Jul 29, 2024
Merged

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Jul 26, 2024

updating github action to run git diff to check uncommitted files. The PR author sometimes forgot to push all the changes required as part of the PR. this causes inconsistency problems with the repo and local. This PR adds a check in the CI and also provides the make target to that developers can ensure that they are pushing all the required changes into the PR

@Madhu-1 Madhu-1 force-pushed the add-checks branch 4 times, most recently from 7096adf to 8e8757d Compare July 26, 2024 10:34
updating github action to run
git diff to check uncommitted files.

Signed-off-by: Madhu Rajanna <[email protected]>
pushing the uncommitted files
to the repo.

Signed-off-by: Madhu Rajanna <[email protected]>
@nb-ohad
Copy link
Collaborator

nb-ohad commented Jul 28, 2024

@Madhu-1 What is the intention behind this action? The current description doesn't seem to provide any information about the problem you are trying to solve.

Where and how would uncommitted changes get into a CI run? Is that supposed to run locally? on Github? What am I missing?

Also, why do we have changes to openAPI CRD definition files in this PR? Is that on purpose? and mistake?
If this PR is trying to accomplish two unrelated goals we should separate the content into two different PRs

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Jul 29, 2024

@Madhu-1 What is the intention behind this action? The current description doesn't seem to provide any information about the problem you are trying to solve.

Where and how would uncommitted changes get into a CI run? Is that supposed to run locally? on Github? What am I missing?

Also, why do we have changes to openAPI CRD definition files in this PR? Is that on purpose? and mistake? If this PR is trying to accomplish two unrelated goals we should separate the content into two different PRs

@nb-ohad after merging the last PR some changes are getting generated locally after running make build This is not expected, This is a check to ensure that the PR contains all the changes that are required not no changes left behind. For example check this action result https://github.com/ceph/ceph-csi-operator/actions/runs/10109935774/job/27958828177

@Madhu-1 Madhu-1 requested a review from nb-ohad July 29, 2024 05:55
@nb-ohad
Copy link
Collaborator

nb-ohad commented Jul 29, 2024

@Madhu-1 What is the intention behind this action? The current description doesn't seem to provide any information about the problem you are trying to solve.
Where and how would uncommitted changes get into a CI run? Is that supposed to run locally? on Github? What am I missing?
Also, why do we have changes to openAPI CRD definition files in this PR? Is that on purpose? and mistake? If this PR is trying to accomplish two unrelated goals we should separate the content into two different PRs

@nb-ohad after merging the last PR some changes are getting generated locally after running make build This is not expected, This is a check to ensure that the PR contains all the changes that are required not no changes left behind. For example check this action result https://github.com/ceph/ceph-csi-operator/actions/runs/10109935774/job/27958828177

@Madhu-1
I believe I understand now. This will block merge if the CI observes that running make build and/or make lint-fix, generates artifacts that are not parts of the code changes committed as part of the PR.
That makes sense.

@nb-ohad nb-ohad merged commit 999bbf2 into ceph:main Jul 29, 2024
7 checks passed
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.

2 participants