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

Add scripts to verify generated codes and Go Modules #1999

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

tenzen-y
Copy link
Member

@tenzen-y tenzen-y commented Nov 3, 2022

Signed-off-by: tenzen-y yuki.iwai.tz@gmail.com

What this PR does / why we need it:
I added scripts to generate Go mock files to Makefile.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #59

Checklist:

  • Docs included if any changes are user facing

@coveralls
Copy link

coveralls commented Nov 4, 2022

Coverage Status

Coverage increased (+1.0%) to 73.419% when pulling 4ff0e5c on tenzen-y:add-mockgen-to-makefile into 54b020b on kubeflow:master.

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thanks for taking this @tenzen-y!

I think to cover this issue: #59 we also want to add unit test to verify that contributor runs make generate before creating PR.

Should we update our unit tests to verify that source code doesn't have any changes after running make generate ?

Makefile Show resolved Hide resolved
@tenzen-y
Copy link
Member Author

tenzen-y commented Nov 14, 2022

Thanks for taking this @tenzen-y!

I think to cover this issue: #59 we also want to add unit test to verify that contributor runs make generate before creating PR.

Should we update our unit tests to verify that source code doesn't have any changes after running make generate ?

Thanks for the review!

Does that mean we add scripts to Makefile or create the shellscript file like the following?

- name: Run Generate And Go Format Test
run: |
go mod download &&
make check &&
git add pkg/apis hack/gen-python-sdk &&
git diff --cached --exit-code || (echo 'Please run "make check" to generate codes and to format Go codes' && exit 1)

@andreyvelich
Copy link
Member

@tenzen-y It's a good idea to re-use this unit test to check if contributor forgot to run make check or go mod tidy.
Should we add more files in addition to (git add pkg/apis hack/gen-python-sdk) to check if make generate needs to be ran ?

@tenzen-y
Copy link
Member Author

@tenzen-y It's a good idea to re-use this unit test to check if contributor forgot to run make check or go mod tidy. Should we add more files in addition to (git add pkg/apis hack/gen-python-sdk) to check if make generate needs to be ran ?

It makes sense.

@google-oss-prow google-oss-prow bot added size/M and removed size/S labels Nov 17, 2022

cd "$(dirname "$0")/.."

git diff --exit-code -- 'pkg/apis' 'hack/gen-python-sdk' 'sdk/python' 'pkg/mock' > /dev/null || \
Copy link
Member Author

Choose a reason for hiding this comment

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

@andreyvelich Are there any other directories that need to be checked?

Copy link
Member

Choose a reason for hiding this comment

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

@tenzen-y What do you think about verifying the whole code base on the changes
(e.g. git diff --exit-code -- . )?
For example, tomorrow we are going to introduce script check to automatically sync operator manifests with our manifests and we ask users to run this script before submitting PR.

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 makes sense.
I will change so that developer can check all files.

Comment on lines 23 to 26
GO_VERSION="$(grep '^go' go.mod | cut -d ' ' -f 2)"
go mod tidy -go "${GO_VERSION}"
git diff --exit-code -- 'go.*' > /dev/null || \
(echo "Please run \"go mod tidy -go ${GO_VERSION}\" to sync Go modules" && 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.

Should we keep code generation check in 1 hack script ?
Then, we can reduce our number of scripts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, thanks!


cd "$(dirname "$0")/.."

git diff --exit-code -- 'pkg/apis' 'hack/gen-python-sdk' 'sdk/python' 'pkg/mock' > /dev/null || \
Copy link
Member

Choose a reason for hiding this comment

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

@tenzen-y What do you think about verifying the whole code base on the changes
(e.g. git diff --exit-code -- . )?
For example, tomorrow we are going to introduce script check to automatically sync operator manifests with our manifests and we ask users to run this script before submitting PR.

Signed-off-by: tenzen-y <yuki.iwai.tz@gmail.com>
@tenzen-y
Copy link
Member Author

@andreyvelich I addressed your suggestions. PTAL

@tenzen-y tenzen-y changed the title Add scripts to generate Go mock files to Makefile Add scripts to verify generated codes and Go Modules Nov 28, 2022
@andreyvelich
Copy link
Member

Thanks for the updates @tenzen-y!
/lgtm
/assign @johnugeorge

@johnugeorge
Copy link
Member

/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge, tenzen-y

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

@google-oss-prow google-oss-prow bot merged commit 6b54eb2 into kubeflow:master Nov 30, 2022
@tenzen-y tenzen-y deleted the add-mockgen-to-makefile branch November 30, 2022 19:03
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.

[test] Add verify-mockgen in CI
4 participants