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

Replace custom codegen package with gengo #9632

Merged
merged 8 commits into from
Jul 29, 2020

Conversation

johngmyers
Copy link
Member

@johngmyers johngmyers commented Jul 27, 2020

This uncovered that the OpenStack SecurityGroupRule task had hand-edited "generated" code. My attempt to fix it may well be incorrect.

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 27, 2020
@k8s-ci-robot k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/digitalocean Issues or PRs related to digitalocean provider area/provider/openstack Issues or PRs related to openstack provider area/provider/spotinst Issues or PRs related to spotinst provider labels Jul 27, 2020
@johngmyers johngmyers force-pushed the gengo branch 3 times, most recently from fbe60ea to 4ed63e5 Compare July 27, 2020 07:39
@johngmyers
Copy link
Member Author

The verify-gomod failure has me stumped.

@hakman
Copy link
Member

hakman commented Jul 27, 2020

I guess this solves the mystery of why dependencies were flaky with launch templates. 😄

@hakman
Copy link
Member

hakman commented Jul 27, 2020

The verify-gomod failure has me stumped.

make gomod contains a few workaround and replaces go mod vendor. You should be ok just running that and pushing the changes. I did a quick test in #9634 and seems fine.

kops/Makefile

Lines 364 to 373 in 26aa335

.PHONY: gomod
gomod: gomod-prereqs
GO111MODULE=on go mod vendor
# Switch weavemesh to use peer_name_hash - bazel rule-go doesn't support build tags yet
rm vendor/github.com/weaveworks/mesh/peer_name_mac.go
sed -i -e 's/peer_name_hash/!peer_name_mac/g' vendor/github.com/weaveworks/mesh/peer_name_hash.go
# Remove all bazel build files that were vendored and regenerate (we assume they are go-gettable)
find vendor/ -name "BUILD" -delete
find vendor/ -name "BUILD.bazel" -delete
make gazelle

@johngmyers johngmyers force-pushed the gengo branch 2 times, most recently from 3c92afd to b82709f Compare July 27, 2020 15:33
@johngmyers
Copy link
Member Author

I'm not confident of my change to SecurityGroupRule. The requirement appears to be for every Task to have a unique value in a field called Name. I'd appreciate a PR before this one that adds that field and modifies all the code that creates a SecurityGroupRule to supply it.

@zetaab @olemarkus

@johngmyers
Copy link
Member Author

Since the SecurityGroupRule boilerplate was hand-edited, moved it out of the _fitask.go file.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2020
@rdrgmnzs
Copy link
Contributor

This is great, thank you @johngmyers !

/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 Jul 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, rdrgmnzs

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:
  • OWNERS [johngmyers,rdrgmnzs]

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

@hakman
Copy link
Member

hakman commented Jul 29, 2020

In case @johngmyers wants some feedback from @zetaab or @olemarkus .
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2020
@johngmyers
Copy link
Member Author

I moved the hand-edited SecurityGroupRole boilerplate out of scope, so I no longer need feedback from @zetaab or @olemarkus. It can either remain out of scope or be moved back to being autogenerated in a later PR.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2020
@k8s-ci-robot k8s-ci-robot merged commit 67966d5 into kubernetes:master Jul 29, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jul 29, 2020
@johngmyers johngmyers deleted the gengo branch July 29, 2020 05:11
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. area/provider/alicloud Issues or PRs related to alicloud provider area/provider/aws Issues or PRs related to aws provider area/provider/digitalocean Issues or PRs related to digitalocean provider area/provider/openstack Issues or PRs related to openstack provider area/provider/spotinst Issues or PRs related to spotinst provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants