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.

Sign up for GitHub

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

Jump to bottom

✨ use Deployment for v2 scaffolding #727

Merged
merged 2 commits into from Jun 5, 2019
Merged

✨ use Deployment for v2 scaffolding #727

merged 2 commits into from Jun 5, 2019

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented May 21, 2019 •
edited

  • 1st commit:
    v2 scaffolding: deployment w/ leader election (add necessary marks for rbac and change Makefile)
    v1 scaffolding (no changes): statefulSet w/o leader election
  • 2nd commit:
    fix a singular|plural noun bug introduced in ⚠️ controller-tools v0.2.0 scaffolding update #682 that causes it to not be able to locate a patch.
  • 3rd commit:
    improve webhook scaffolding by re-org kustomize config and variable and better comments

close #535

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 21, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mengqiy

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 May 21, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 21, 2019
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 22, 2019
@mengqiy mengqiy changed the title [WIP] ✨ use Deployment for v2 scaffolding ✨ use Deployment for v2 scaffolding May 22, 2019
@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 May 22, 2019
@mengqiy
Copy link
Member Author

mengqiy commented May 22, 2019

This is ready for review.

droot
droot reviewed May 22, 2019
View reviewed changes
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

I have a few comments.

pkg/scaffold/v2/makefile.go Show resolved Hide resolved
pkg/scaffold/v2/main.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/main.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/crd/enablewebhook_patch.go Outdated Show resolved Hide resolved
@mengqiy mengqiy mentioned this pull request May 28, 2019
Closed
@DirectXMan12 DirectXMan12 added this to the v2.0.0 milestone May 29, 2019
@DirectXMan12 DirectXMan12 moved this from Code to Code Awaiting Review in 2.0.0 cross-project tracking May 29, 2019
DirectXMan12
DirectXMan12 requested changes May 30, 2019
View reviewed changes
pkg/scaffold/v2/main.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/main.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/manager/config.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/main.go Outdated Show resolved Hide resolved
@DirectXMan12
Copy link
Contributor

FWIW, I'm not certain we actually need to switch (what's the actual benefit here?). Turning on leader election is still useful though.

@mengqiy
Copy link
Member Author

mengqiy commented May 31, 2019

FWIW, I'm not certain we actually need to switch (what's the actual benefit here?). Turning on leader election is still useful though.

For the most common case (stateless controller), people don't get more benefits by using StatefulSet instead of Deployment when leader election is enabled. And using StatefulSet actually confuses the users, I saw multiple users confused by this before.
For the uncommon case (stateful controller), we document it in the book how to use StatefulSet.

@mengqiy mengqiy force-pushed the deploymentwithLE branch 2 times, most recently from d8721da to 5881075 Compare June 4, 2019 18:40
@mengqiy
Copy link
Member Author

mengqiy commented Jun 4, 2019

Addressed comments and rebased to resolve conflicts.
PTAL

droot
droot reviewed Jun 5, 2019
View reviewed changes
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Looks good. Have a few minor comments.

pkg/scaffold/v2/crd/kustomization.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/crd/kustomization.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/main.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/staticrole.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/manager/config.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/staticrole.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/manager/config.go
@@ -76,9 +76,7 @@ spec:
matchLabels:
control-plane: controller-manager
controller-tools.k8s.io: "1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this annotation from all manifests ?

Copy link
Member Author

Choose a reason for hiding this comment

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

controller-tools.k8s.io: "1.0"

We can definitely remove these labels. But this may end up causing we duplicate almost all of the shared files, because most manifests files have that label. So I'd like to do it in a followup PR, since it's a cleanup.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #764 to track

@ymmt2005 ymmt2005 mentioned this pull request Jun 5, 2019
Merged
@mengqiy
Copy link
Member Author

mengqiy commented Jun 5, 2019

I will squash commits before merging. So putting a hold label for now.
/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 Jun 5, 2019
@DirectXMan12
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2019
@mengqiy mengqiy mentioned this pull request Jun 5, 2019
Closed
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2019
@mengqiy
Copy link
Member Author

mengqiy commented Jun 5, 2019

Squashed commits
/hold cancel
No code changes. Re-applying label per #727 (comment)

@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 Jun 5, 2019
@mengqiy mengqiy added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2019
@k8s-ci-robot k8s-ci-robot merged commit 41ed225 into kubernetes-sigs:master Jun 5, 2019
2.0.0 cross-project tracking automation moved this from Code Awaiting Review to Done Jun 5, 2019
@mengqiy mengqiy deleted the deploymentwithLE branch June 5, 2019 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DirectXMan12 DirectXMan12 DirectXMan12 requested changes

@droot droot droot left review comments

@pwittrock pwittrock Awaiting requested review from pwittrock

Assignees

@DirectXMan12 DirectXMan12

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
No open projects
2.0.0 cross-project tracking
  
Done
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.

Running controller manager as Deployment
4 participants
@mengqiy @k8s-ci-robot @DirectXMan12 @droot