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 verify scripts for make gen #483

Closed
damemi opened this issue Jan 12, 2021 · 9 comments · Fixed by #507
Closed

Add verify scripts for make gen #483

damemi opened this issue Jan 12, 2021 · 9 comments · Fixed by #507
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@damemi
Copy link
Contributor

damemi commented Jan 12, 2021

Issues such as the one described in #482 could be avoided if we add automatic verify scripts on PRs to ensure that make gen (and its subscripts) have been run. It should be simple enough as running make gen in a tmp dir and checking if any changes have been generated.

/kind cleanup
/help

@k8s-ci-robot
Copy link
Contributor

@damemi:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

Issues such as the one described in #482 could be avoided if we add automatic verify scripts on PRs to ensure that make gen (and its subscripts) have been run. It should be simple enough as running make gen in a tmp dir and checking if any changes have been generated.

/kind cleanup
/help

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.

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Jan 12, 2021
@pravarag
Copy link
Contributor

pravarag commented Feb 15, 2021

@damemi I would like to work on this issue if it's not already been taken :)

/assign

@pravarag
Copy link
Contributor

@damemi I've come across few doubts while trying to understand the issue. As you mentioned that we need this change to ensure make gen has run it's all subscripts proprerly and when I try to run make gen locally, it basically generates three binaries,

conversion-gen
deepcopy-gen
defaulter-gen

So, do we need to ensure that make gen has generated those three binaries in /tmp location properly or not?

@damemi
Copy link
Contributor Author

damemi commented Feb 18, 2021

Those binaries are what actually run to generate the updated code. make gen conveniently downloads them (if they do not already exist) and stores them in the tmp directory which is ignored by git.

The goal here is to make sure that code which is about to be committed has already run these generators. What we want to check is that the diff in a PR doesn't change when you run make gen against it.

So, since the binaries themselves are not committed we can't check that they are stored in /tmp (but if they aren't being downloaded, then the generators won't be running anyway, so you will still see a diff).

@seanmalloy
Copy link
Member

Not quite done yet.

/reopen

@k8s-ci-robot
Copy link
Contributor

@seanmalloy: Reopened this issue.

In response to this:

Not quite done yet.

/reopen

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
Copy link
Contributor

@seanmalloy @damemi is it okay to close this issue now? The last PR for defaulters gen has been merged today. Let me know if there is anything else I need to check w.r.t this issue.

@seanmalloy
Copy link
Member

I believe everything is done now. @pravarag thanks for your help with this!

/close

@k8s-ci-robot
Copy link
Contributor

@seanmalloy: Closing this issue.

In response to this:

I believe everything is done now. @pravarag thanks for your help with this!

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants