Add verify scripts for make gen to run during PR

[pravarag]

This PR fixes: #483

As part of this change, we want to add steps to ensure that make gen has been run and there are no changes being generated during the PR.
For now, I've tested the code in local for the steps which I've added in the Makefile and below is the output for same,

 ->  make verify-gen
./hack/verify-makegen.sh

Diff in the PR has changed
make: *** [Makefile:112: verify-gen] Error 1

Since it's reporting error, which means it's pointing that the generators have been downloaded at the latest in the _output directory in my local.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Details

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.

[k8s-ci-robot]

Welcome @pravarag!

It looks like this is your first PR to kubernetes-sigs/descheduler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/descheduler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @pravarag. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[pravarag]

I signed it

[damemi]

thanks for the contribution @pravarag! This is a good start, some comments below

[damemi]

This script should do more than just check for a git diff, it should ideally actually run the generator scripts in a temp directory and compare that to the current output. The reason for this is because this script on its own isn't verifying make gen, so if I call it directly with ./hack/verify-makegen.sh or make verify-gen, I won't really be checking that.

You can see how verify-vendor.sh works for an example.

Following that pattern, I think it would be better to have separate ./hack/verify-xx... scripts for each of the generators (for the same reason: that I may want to just verify one of them).

[pravarag]

Hi @damemi thanks for pointing out the reference script verify-vendor.sh. I wanted to check, is this the line which suggests correct way of using git diff : L64 ? Also, in our case for the git diff to compare, do we need to take output generated by ./hack/updated-generated-xxx scripts in the latest commit with the output generated in the temp_ directory( created during PR check) ?

[damemi]

I was referring more to how this script is pushing the existing changes onto a tmp directory, running the generator, and then comparing if any new changes occurred. So yes, the diff should be comparing the latest commit to a copy of the latest commit that has had the generator run. In this case, you are just getting that by doing git diff on the copy

[damemi]

nit: newline at end of file

[damemi]

/ok-to-test
/refresh

[WillsonHG]

/check-cla

[pravarag]

@damemi also, I've come across a doubt during local execution. When I run make gen in my local I see the below output,

->  make gen
./hack/update-generated-conversions.sh
./hack/update-generated-deep-copies.sh
./hack/update-generated-defaulters.sh

But, when I explicitly run this command L4, I'm getting below error as well,

->  ./conversion-gen "k8s.io/code-generator/cmd/conversion-gen"
F0228 21:55:13.353319    8843 conversion.go:204] Failed loading boilerplate: open /Users/pravaragrawal/Work/go_work/src/k8s.io/code-generator/hack/boilerplate.go.txt: no such file or directory
goroutine 1 [running]:

Is this error something which is to be ignored at this point?

[damemi]

"k8s.io/code-generator/cmd/conversion-gen"

@pravarag the command you linked to is a go build command that is attempting to compile the conversion-gen binary. It looks like you are running it as a command itself, which is trying to run conversion-gen with its own package as a parameter.

So yes, you can ignore this error. In general, trying to run these bash scripts line-by-line may not work very well (due to imports of other init files setting variables etc). You should just be running make gen, or the individual gen targets/scripts

[damemi]

@pravarag you might also be able to take a look at the PR I just opened to add a similar verify script for the table of contents: #516

That could help you on these other generators. We could also probably move some of the redundant code to a lib file so it isn't run over and over, but that's not vital.

A problem I found is that the files from _output (ie, our generator binaries) aren't copied to the temp directory with git archive (since they are .gitignore'd). So I had to manually copy that directory like https://github.com/kubernetes-sigs/descheduler/pull/516/files#diff-d257d22e73af313e0d4311fccbdd5054c2affab762a1d2965fd68217f0331963R36

Hope this helps

[pravarag]

A problem I found is that the files from _output (ie, our generator binaries) aren't copied to the temp directory with git archive (since they are .gitignore'd). So I had to manually copy that directory like https://github.com/kubernetes-sigs/descheduler/pull/516/files#diff-d257d22e73af313e0d4311fccbdd5054c2affab762a1d2965fd68217f0331963R36

Thanks @damemi , I'll check the changes done in mentioned PR.

[pravarag]

@damemi I've made some changes and pushed the latest code taking reference from the script you suggested in previous comments. But, I keep getting bad substitution error on this line and I'm guessing it's something to do with how diff _output is being used. So, one doubt here - since in this case as when we run the generators, I don't see a file other than the ones generated in _output directory which may get changed unlike README.md in for this change. Hence, which two files I must be comparing here to check the diff output and proceed based on that?

[damemi]

@pravarag the script shouldn't be running diff _output, because _output is just the temporary directory where the generator binaries are downloaded to.

If you see this line in verify-vendor.sh, it is calling:

diff -Naupr --ignore-matching-lines='^\s*\"GoVersion\":' go.mod "${_deschedulertmp}/go.mod"

which is essentially

diff go.mod {tmpdir}/go.mod

Because when we update the vendor, we expect changes to go.mod.

If you are running the generators for this PR without any significant changes to the code (for example, modifying the API) then you will not see any generated changes besides downloading the binaries, as you said. We can ignore anything that happens in _output because we do not commit that directory to the Github repo.

As an example, if you check out this commit from my fork and run make gen, you'll see that it adds the following to pkg/api/v1alpha1/zz_generated.conversion.go:

diff --git a/pkg/api/v1alpha1/zz_generated.conversion.go b/pkg/api/v1alpha1/zz_generated.conversion.go
index 727b9c776..8f56bf309 100644
--- a/pkg/api/v1alpha1/zz_generated.conversion.go
+++ b/pkg/api/v1alpha1/zz_generated.conversion.go
@@ -151,6 +151,7 @@ func autoConvert_v1alpha1_DeschedulerStrategy_To_api_DeschedulerStrategy(in *Des
        out.Enabled = in.Enabled
        out.Weight = in.Weight
        out.Params = (*api.StrategyParameters)(unsafe.Pointer(in.Params))
+       out.Foo = in.Foo
        return nil
 }
 
@@ -163,6 +164,7 @@ func autoConvert_api_DeschedulerStrategy_To_v1alpha1_DeschedulerStrategy(in *api
        out.Enabled = in.Enabled
        out.Weight = in.Weight
        out.Params = (*StrategyParameters)(unsafe.Pointer(in.Params))
+       out.Foo = in.Foo
        return nil
 }

So, if you imagine a PR that just has the above commit (and did not run make gen), then your verify script would do a couple things when it runs in CI:

1. Copy the current entire descheduler code directory to a {tmpdir}
2. pushd into the {tmpdir} (should be something like _tmp/kube-vendor.ZhSdEN/descheduler if you are running locally)
3. Run make gen (or each individual script) inside the {tmpdir}
4. popd out to the real directory
5. Compare the current directory to the tmpdir with something like diff -Naupr pkg/ {deschedulertmp}/descheduler/pkg/ (any generated changes will just be under pkg/)

[pravarag]

@pravarag I did not look into the seg fault error in detail. It sound like only the verify-defaulters.sh script is causing the seg fault. One simple idea would be to do multiple smaller PRs. You could submit a separate PR to only add the hack/verify-conversions.sh script. Then if that works do another PR to add hack/verify-deep-copies.sh.

Thanks @seanmalloy , as you suggested I've shortened this PR to include changes for verify-conversions.sh only. And if this goes well, I'll create two other PRs for verify-deep-copies.sh & verify-defaulters.sh.

[damemi]

Hi @pravarag, thanks for working on this so far. I agree that we can just merge the verify checks for the 2 scripts we know are working and continue to debug why defaulters fails like that. Thanks for sticking with this 🙂
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, pravarag

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [damemi]

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

[pravarag]

Thanks @damemi , it was all your help :) and it's great to see my first PR coming towards a closure. Also, this PR only consists changes for verify-conversions so do you want me to include changes for verify-deep-copies as well along with this PR and take verify-defaulters separately? Or like how @seanmalloy mentioned above, three PRs for respective three generators?

[seanmalloy]

three PRs for respective three generators

I think we can do three separate PRs. One for each generator.

[seanmalloy]

So the automated CI checks all work. But I get the below error when running make verify-gen from my laptop. I get the same error on Mac OSX and Linux.

$ make verify-gen
ls: cannot access '_output/bin/golangci-lint': No such file or directory
./hack/verify-conversions.sh
mktemp: failed to create directory via template ‘./hack/../_tmp/kube-verify.XXXXXX’: No such file or directory
make: *** [verify-gen] Error 1

@pravarag were you able to successfully run make verify-gen from your local computer? Am I doing something wrong?

[pravarag]

@pravarag were you able to successfully run make verify-gen from your local computer? Am I doing something wrong?

@seanmalloy here's an output from my local,

-bash>descheduler(add-verify-scripts)$
 ->  pwd
/Users/pravaragrawal/Work/go_work/src/github.com/pravarag/descheduler
-bash>descheduler(add-verify-scripts)$
 ->  make verify-gen
./hack/verify-conversions.sh
Generated conversions verified.
-bash>descheduler(add-verify-scripts)$
 ->  ls -latr
total 156
drwxr-xr-x  4 pravaragrawal staff   128 Feb 15 19:57 .github
-rw-r--r--  1 pravaragrawal staff  1591 Feb 15 19:57 CONTRIBUTING.md
-rw-r--r--  1 pravaragrawal staff 11357 Feb 15 19:57 LICENSE
-rw-r--r--  1 pravaragrawal staff   617 Feb 15 19:57 SECURITY_CONTACTS
drwxr-xr-x  3 pravaragrawal staff    96 Feb 15 19:57 charts
-rw-r--r--  1 pravaragrawal staff   886 Feb 15 19:57 cloudbuild.yaml
drwxr-xr-x  3 pravaragrawal staff    96 Feb 15 19:57 cmd
-rw-r--r--  1 pravaragrawal staff   148 Feb 15 19:57 code-of-conduct.md
drwxr-xr-x  6 pravaragrawal staff   192 Feb 15 19:57 examples
drwxr-xr-x  5 pravaragrawal staff   160 Feb 15 19:57 kubernetes
drwxr-xr-x  3 pravaragrawal staff    96 Feb 15 19:58 _output
drwxr-xr-x  6 pravaragrawal staff   192 Mar  8 20:22 ..
-rw-r--r--  1 pravaragrawal staff   200 Mar 14 12:38 .golangci.yml
-rw-r--r--  1 pravaragrawal staff    65 Apr  4 10:16 .gitignore
-rw-r--r--  1 pravaragrawal staff   949 Apr  4 10:16 Dockerfile
-rw-r--r--  1 pravaragrawal staff   777 Apr  4 10:16 Dockerfile.dev
-rw-r--r--  1 pravaragrawal staff   219 Apr  4 10:16 OWNERS
drwxr-xr-x  6 pravaragrawal staff   192 Apr  4 10:16 docs
-rw-r--r--  1 pravaragrawal staff   432 Apr  4 10:16 go.mod
-rw-r--r--  1 pravaragrawal staff 65877 Apr  4 10:16 go.sum
drwxr-xr-x  3 pravaragrawal staff    96 Apr  4 10:16 metrics
drwxr-xr-x  7 pravaragrawal staff   224 Apr  4 10:16 pkg
drwxr-xr-x 12 pravaragrawal staff   384 Apr  4 10:16 vendor
-rw-r--r--  1 pravaragrawal staff  4338 Apr  7 08:58 Makefile
drwxr-xr-x 30 pravaragrawal staff   960 Apr  7 08:58 .
-rw-r--r--  1 pravaragrawal staff 25846 Apr  7 08:58 README.md
drwxr-xr-x 19 pravaragrawal staff   608 Apr  7 08:58 hack
drwxr-xr-x  6 pravaragrawal staff   192 Apr  7 08:58 test
drwxr-xr-x 70 pravaragrawal staff  2240 Apr  7 08:58 _tmp
drwxr-xr-x 16 pravaragrawal staff   512 Apr  7 08:58 .git

I'm not getting the error which you mentioned above with my current clone of the repository. But, I'm not sure if it's not coming since I already have the working-repository setup. I'll try to check by doing a fresh clone as well.

[pravarag]

Okay, I was getting the similar error like @seanmalloy mentioned above in my local(OSX) when I did a fresh clone of the fork:

-bash>descheduler((HEAD detached at origin/add-verify-scripts))$
 ->  make verify-gen
ls: cannot access '_output/bin/golangci-lint': No such file or directory
./hack/verify-conversions.sh
mktemp: failed to create directory via template ‘./hack/../_tmp/kube-verify.XXXXXX’: No such file or directory
make: *** [Makefile:124: verify-gen] Error 1

And I found the culprit, it's a missing mkdir command before this line. After adding the same, it works fine without any error,

-bash>descheduler((HEAD detached at origin/add-verify-scripts))$
 ->  make verify-gen
ls: cannot access '_output/bin/golangci-lint': No such file or directory
./hack/verify-conversions.sh
Generated conversions verified.

I'll make changes and push the code to working branch then.

[damemi]

Awesome work @pravarag! Could you please squash this down to 1 commit?
/hold
for squash

[pravarag]

Awesome work @pravarag! Could you please squash this down to 1 commit?

Squashed the commits with the latest push @damemi 🙂

[damemi]

/hold cancel

[seanmalloy]

The script always exits zero. See inline comments for suggested fix.

[seanmalloy]

Suggested change

	echo "Generated conversions verify failed."
	echo "Generated conversions verify failed."
	exit 1

[pravarag]

Thanks @seanmalloy, I've made the change with latest push.

[seanmalloy]

Suggested change

	popd > /dev/null 2>&1
	ret=$?
	unset PRJ_PREFIX
	if [[ ${ret} -ne 0 ]]; then
	exit ${ret}
	fi

[pravarag]

Updated the review comments.

[seanmalloy]

/lgtm

[pravarag]

Thanks @seanmalloy @damemi 🙂