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

refactor/ci tasks improvement new #5303

Merged

Conversation

antoooks
Copy link
Contributor

@antoooks antoooks commented Aug 31, 2023

  • fix generate-kustomize-builtin-plugins
  • add diff checker on generate-kustomize-builtin-plugins
  • add go mod tidy and go work sync on presubmit check
  • add generate-kustomize-builtin-plugins on presubmit check
  • ensure pluginator logic does not create diff
  • add comments

addresses #4977

@k8s-ci-robot
Copy link
Contributor

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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 the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2023
@k8s-ci-robot k8s-ci-robot requested review from koba1t and yuwenma August 31, 2023 09:45
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 31, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @antoooks. 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.

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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 31, 2023
… generate-kustomize-builtin-plugins, add go work sync on presubmit check, add generate-kustomize-builtin-plugins on presubmit check
@antoooks antoooks force-pushed the refactor/ci-tasks-improvement-new branch from d9b088d to 8f28349 Compare September 16, 2023 14:47
@antoooks antoooks marked this pull request as ready for review September 16, 2023 17:25
@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 Sep 16, 2023
@antoooks antoooks closed this Sep 16, 2023
@antoooks antoooks reopened this Sep 16, 2023
@antoooks
Copy link
Contributor Author

/assign @koba1t @Debanitrkl

@koba1t
Copy link
Member

koba1t commented Sep 17, 2023

Hi @antoooks
Thanks for your contribution!

make generate-kustomize-builtin-plugins command failed on my computer when I was testing your PR.
Are you successfully running for your computer?

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@antoooks
Copy link
Contributor Author

antoooks commented Sep 18, 2023

Hi @antoooks Thanks for your contribution!

make generate-kustomize-builtin-plugins command failed on my computer when I was testing your PR. Are you successfully running for your computer?

Yes it worked in mine, could you paste me some of the error messages? 😃

@koba1t
Copy link
Member

koba1t commented Sep 18, 2023

@antoooks

could you paste me some of the error messages?

The following error message appears. Do you get this error?

$ make generate-kustomize-builtin-plugins
for plugin in /Users/kob/git/kustomize/plugin/builtin/annotationstransformer /Users/kob/git/kustomize/plugin/builtin/configmapgenerator /Users/kob/git/kustomize/plugin/builtin/hashtransformer /Users/kob/git/kustomize/plugin/builtin/helmchartinflationgenerator /Users/kob/git/kustomize/plugin/builtin/iampolicygenerator /Users/kob/git/kustomize/plugin/builtin/imagetagtransformer /Users/kob/git/kustomize/plugin/builtin/labeltransformer /Users/kob/git/kustomize/plugin/builtin/namespacetransformer /Users/kob/git/kustomize/plugin/builtin/patchjson6902transformer /Users/kob/git/kustomize/plugin/builtin/patchstrategicmergetransformer /Users/kob/git/kustomize/plugin/builtin/patchtransformer /Users/kob/git/kustomize/plugin/builtin/prefixtransformer /Users/kob/git/kustomize/plugin/builtin/replacementtransformer /Users/kob/git/kustomize/plugin/builtin/replicacounttransformer /Users/kob/git/kustomize/plugin/builtin/resourcegenerator /Users/kob/git/kustomize/plugin/builtin/secretgenerator /Users/kob/git/kustomize/plugin/builtin/sortordertransformer /Users/kob/git/kustomize/plugin/builtin/suffixtransformer /Users/kob/git/kustomize/plugin/builtin/valueaddtransformer; do \
                echo "generating ${plugin} ..."; \
                set -e; \
                cd ${plugin}; \
                go generate pluginator .; \
        done; \
        cd ../../../; \
        make no-diff \

generating /Users/kob/git/kustomize/plugin/builtin/annotationstransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/configmapgenerator ...
generating /Users/kob/git/kustomize/plugin/builtin/hashtransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/helmchartinflationgenerator ...
generating /Users/kob/git/kustomize/plugin/builtin/iampolicygenerator ...
generating /Users/kob/git/kustomize/plugin/builtin/imagetagtransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/labeltransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/namespacetransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/patchjson6902transformer ...
generating /Users/kob/git/kustomize/plugin/builtin/patchstrategicmergetransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/patchtransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/prefixtransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/replacementtransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/replicacounttransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/resourcegenerator ...
go: cannot find main module, but found .git/config in /Users/kob/git/kustomize
        to create a module there, run:
        cd ../../.. && go mod init
make: *** [generate-kustomize-builtin-plugins] Error 1

@antoooks
Copy link
Contributor Author

@antoooks

could you paste me some of the error messages?

The following error message appears. Do you get this error?

$ make generate-kustomize-builtin-plugins
for plugin in /Users/kob/git/kustomize/plugin/builtin/annotationstransformer /Users/kob/git/kustomize/plugin/builtin/configmapgenerator /Users/kob/git/kustomize/plugin/builtin/hashtransformer /Users/kob/git/kustomize/plugin/builtin/helmchartinflationgenerator /Users/kob/git/kustomize/plugin/builtin/iampolicygenerator /Users/kob/git/kustomize/plugin/builtin/imagetagtransformer /Users/kob/git/kustomize/plugin/builtin/labeltransformer /Users/kob/git/kustomize/plugin/builtin/namespacetransformer /Users/kob/git/kustomize/plugin/builtin/patchjson6902transformer /Users/kob/git/kustomize/plugin/builtin/patchstrategicmergetransformer /Users/kob/git/kustomize/plugin/builtin/patchtransformer /Users/kob/git/kustomize/plugin/builtin/prefixtransformer /Users/kob/git/kustomize/plugin/builtin/replacementtransformer /Users/kob/git/kustomize/plugin/builtin/replicacounttransformer /Users/kob/git/kustomize/plugin/builtin/resourcegenerator /Users/kob/git/kustomize/plugin/builtin/secretgenerator /Users/kob/git/kustomize/plugin/builtin/sortordertransformer /Users/kob/git/kustomize/plugin/builtin/suffixtransformer /Users/kob/git/kustomize/plugin/builtin/valueaddtransformer; do \
                echo "generating ${plugin} ..."; \
                set -e; \
                cd ${plugin}; \
                go generate pluginator .; \
        done; \
        cd ../../../; \
        make no-diff \

generating /Users/kob/git/kustomize/plugin/builtin/annotationstransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/configmapgenerator ...
generating /Users/kob/git/kustomize/plugin/builtin/hashtransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/helmchartinflationgenerator ...
generating /Users/kob/git/kustomize/plugin/builtin/iampolicygenerator ...
generating /Users/kob/git/kustomize/plugin/builtin/imagetagtransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/labeltransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/namespacetransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/patchjson6902transformer ...
generating /Users/kob/git/kustomize/plugin/builtin/patchstrategicmergetransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/patchtransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/prefixtransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/replacementtransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/replicacounttransformer ...
generating /Users/kob/git/kustomize/plugin/builtin/resourcegenerator ...
go: cannot find main module, but found .git/config in /Users/kob/git/kustomize
        to create a module there, run:
        cd ../../.. && go mod init
make: *** [generate-kustomize-builtin-plugins] Error 1

@koba1t I have tested by cloning it inside a docker container and I cannot find this plugin/builtin/resourcegenerator module on master or on my branch, I think this is something added when you switched your work tree. I suggest you start cloning clean on a different directory. Then do make uninstall-tools and make install-tools.

@koba1t
Copy link
Member

koba1t commented Sep 18, 2023

@antoooks

Yes, You are right.
My computer has an empty dir named plugin/builtin/resourcegenerator and this empty dir is watched from Makefile script at this line
This command is successful after deleting this empty dir.

Apologies for overlooking the mistake.

@antoooks
Copy link
Contributor Author

@antoooks

Yes, You are right. My computer has an empty dir named plugin/builtin/resourcegenerator and this empty dir is watched from Makefile script at this line This command is successful after deleting this empty dir.

Apologies for overlooking the mistake.

No worries, glad it worked! I also have added all the feedback for workspace-sync step

@rayandas
Copy link
Member

rayandas commented Sep 21, 2023

/ok-to-test
Edit: Ah! Seems like we need a ok-to-test from maintainers.

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 21, 2023
Copy link
Member

@koba1t koba1t left a comment

Choose a reason for hiding this comment

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

Hi, @antoooks!
I added a few review comments. Please check my comments!

Makefile Outdated

# Pushes dependencies in the go.work file back to go.mod files of each workspace module.
.PHONY: workspace-sync
workspace sync:
Copy link
Member

Choose a reason for hiding this comment

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

Do you have any specific reason to remove the hyphen here?
I think it is better to use a unified name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No issue here, yes we can go with unified name

.PHONY: generate-kustomize-builtin-plugins
generate-kustomize-builtin-plugins: $(builtinplugins)
generate-kustomize-builtin-plugins: $(builtplugins)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I think it is better to separate a step of generate and a step of diff check.
Because generate-kustomize-builtin-plugins will be used in developing built-in plugins. This command may error when developing plugins before exec git commit.
I think it better to separate them there, It will be useful for developing plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. Do you know if diff check will be called anywhere else? Since the context is strictly limited to builtin plugins maybe I should use builtin-plugins no diff as target name instead

Copy link
Member

Choose a reason for hiding this comment

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

I imagine making a new builtin-plugins diff command, and that makes called from here.
I'll leave it to you to figure out how to do it.

@koba1t
Copy link
Member

koba1t commented Oct 18, 2023

/assign

@koba1t
Copy link
Member

koba1t commented Oct 19, 2023

@antoooks
Thanks for the fix! I appreciate your contribution!
This PR looks good to me. I'll merge this PR!

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoooks, koba1t

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 19, 2023
@k8s-ci-robot k8s-ci-robot merged commit 752bb2e into kubernetes-sigs:master Oct 19, 2023
5 checks passed
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Development

Successfully merging this pull request may close these issues.

5 participants