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

Update verify scripts to use the generation scripts directly #982

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Oct 13, 2022

In #958 we are noticing odd behavior with the verify tools for our generator scripts, specifically conversions. CI is failing to verify the conversion-gen saying there are changes missing. But, running these generators on some local environments (and containers) produces no diff, while other environments do produce the generated output that the tests are expecting.

96c03a3 and 2b1746c update the generator scripts (deepcopies, conversion, and defaulters) to dynamically parse which files to run on using the function find_dirs_containing_comment_tags. But, it didn't update the verify scripts to parse similarly (they are still hardcoded).

Updating the verify scripts to call the generators directly is a good idea to keep our generators and changes in line. Doing so fixed the verify failures (at least for me locally). But, I don't know if this is actually fixing it, or if there is a bug in the new generator command with find_dirs_containing_comment_tags and this is just copying the bug over to the verify scripts as well.

What looks suspicious to me is this line which replaces . in the path names with ${PRJ_PREFIX}. So on my local for example, the output of this function for conversion-gen is:

sigs.k8s.io/descheduler/pkg/api/v1alpha1,sigs.k8s.io/descheduler/pkg/apis/componentconfig/v1alpha1

as opposed to

./pkg/api/v1alpha1,./pkg/apis/componentconfig/v1alpha1

This stands out because verify-conversions.sh specifically currently has the latter as its input dirs. But, the generator is using the former as its input dirs.

The fact that this shows up differently on different environments makes me think it may be an issue with how PRJ_PREFIX is parsed. Maybe similar to #313. Marking this WIP for now to continue work on it

See also a similar weird output from the deepcopy script in #976 (comment)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 13, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 13, 2022
@damemi
Copy link
Contributor Author

damemi commented Oct 14, 2022

ping @spike-liu, are you able to check find_dirs_containing_comment_tags to see if there is an issue there? It looks like the generator is giving some weird output in #976 (comment) as well

@spike-liu
Copy link
Contributor

ping @spike-liu, are you able to check find_dirs_containing_comment_tags to see if there is an issue there? It looks like the generator is giving some weird output in #976 (comment) as well

Yes, of course. Let me do some research.

@spike-liu
Copy link
Contributor

@damemi Yes, you are right. Previously replacing . with ${PRJ_PREFIX} in find_dirs_containing_comment_tags is for the consistency with the original hardcode. Verified in local test, this line could be removed safely and verify-deep-copies.sh could be run successfully within this PR as below:
image

As for the issue in #976, I found some golang files under _tmp folder generated in verify-xxx.sh would be found mistakenly by find_dirs_containing_comment_tags(). However we could fix with excluding _tmp folders as below:
image

Please feel free to let me know if PR was needed.

@damemi
Copy link
Contributor Author

damemi commented Oct 16, 2022

@spike-liu thanks for debugging this! yes, if you wouldn't mind opening a PR with your fixes it would be really helpful

@spike-liu
Copy link
Contributor

@spike-liu thanks for debugging this! yes, if you wouldn't mind opening a PR with your fixes it would be really helpful

Done with #988

Please feel free to review.

@damemi damemi changed the title [wip] Update verify scripts to use the generation scripts directly Update verify scripts to use the generation scripts directly Oct 17, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2022
@damemi
Copy link
Contributor Author

damemi commented Oct 17, 2022

/cc @knelasevero

@knelasevero
Copy link
Contributor

Nice! This have been bugging me for some time. Great stuff

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2022
Copy link
Member

@JaneLiuL JaneLiuL left a comment

Choose a reason for hiding this comment

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

/lgtm

@damemi
Copy link
Contributor Author

damemi commented Oct 18, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2022
@k8s-ci-robot k8s-ci-robot merged commit 30aab9c into kubernetes-sigs:master Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants