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

Add v1beta2.DaemonSet #49071

Merged
merged 9 commits into from
Jul 26, 2017
Merged

Add v1beta2.DaemonSet #49071

merged 9 commits into from
Jul 26, 2017

Conversation

foxish
Copy link
Contributor

@foxish foxish commented Jul 18, 2017 •
edited
Loading

Depends on #48746
Partly implements #49135

Adding type apps/v1beta2.DaemonSet

@foxish foxish added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 18, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 18, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 18, 2017
@foxish foxish assigned foxish and unassigned smarterclayton and wojtek-t Jul 18, 2017
@vikaschoudhary16
Copy link
Contributor

@foxish Please add a reference to issue/proposal for context.

@foxish foxish force-pushed the foxish-api branch 2 times, most recently from fc0ce8b to 01b7051 Compare July 18, 2017 03:10
@foxish foxish changed the title Add v1beta2.DaemonSet [WIP] Add v1beta2.DaemonSet Jul 18, 2017
@foxish foxish added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. release-note-label-needed labels Jul 18, 2017
@foxish
Copy link
Contributor Author

foxish commented Jul 18, 2017

/test pull-kubernetes-e2e-gce-etcd3

@foxish
Copy link
Contributor Author

foxish commented Jul 18, 2017

Tests expected to fail. No autogenerated changes added. Waiting for #48746 to merge.

@kow3ns kow3ns mentioned this pull request Jul 21, 2017
Closed
11 tasks
@foxish
Copy link
Contributor Author

foxish commented Jul 21, 2017

This is ready for review. PTAL @janetkuo @kow3ns @erictune

janetkuo
janetkuo reviewed Jul 21, 2017
View reviewed changes
staging/src/k8s.io/api/apps/v1beta2/types.go Outdated
@@ -29,6 +29,191 @@ const (
StatefulSetRevisionLabel = ControllerRevisionHashLabelKey
)

type DaemonSetUpdateStrategy struct {
Copy link
Member

Choose a reason for hiding this comment

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

You need to mark these new APIs WIP (see other structs in this file) as we're going to break it. Please put DaemonSet APIs in the roughly same location as extensions/v1beta1 (i.e. after Deployment).

@janetkuo janetkuo assigned janetkuo and unassigned foxish Jul 21, 2017
smarterclayton
smarterclayton reviewed Jul 25, 2017
View reviewed changes
plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go Outdated
@@ -175,7 +175,8 @@ func ClusterRoles() []rbac.ClusterRole {
rbac.NewRule("impersonate").Groups(legacyGroup).Resources("serviceaccounts").RuleOrDie(),

rbac.NewRule(ReadWrite...).Groups(appsGroup).Resources("statefulsets",
"deployments", "deployments/scale", "deployments/rollback").RuleOrDie(),
Copy link
Contributor

Choose a reason for hiding this comment

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

DaemonSets were not in this list before because they could evict other workloads outside of the namespace. @liggitt on intent from sig-auth here

Copy link
Member

Choose a reason for hiding this comment

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

drop the admin/edit/view role change from this PR and open a follow up to discuss with @kubernetes/sig-scheduling-pr-reviews and @kubernetes/sig-node-pr-reviews... I'm not sure of the current state of daemonset-spawned pod priority

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed changes to the roles. Thanks!

@smarterclayton
Copy link
Contributor

/approve

One comment for @kubernetes/sig-auth-pr-reviews re daemonset in namespace admin role

@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Jul 25, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: foxish, janetkuo, smarterclayton

Associated issue: 49135

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 25, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 25, 2017
@foxish
Copy link
Contributor Author

foxish commented Jul 25, 2017

Readding LGTM based on previous.
Latest change addressed rebase conflicts and removed changes to roles.

@foxish foxish added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2017
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 25, 2017 •
edited
Loading

@foxish: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-cross af619e8a39b6808d834374c1fb92984f66dc0bda link /test pull-kubernetes-cross

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@foxish
Copy link
Contributor Author

foxish commented Jul 25, 2017

/test pull-kubernetes-kubemark-e2e-gce
/test pull-kubernetes-e2e-gce-etcd3

@foxish foxish added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2017
@janetkuo janetkuo mentioned this pull request Jul 26, 2017
Closed
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 43443, 46193, 49071, 47252)

@k8s-github-robot k8s-github-robot merged commit 4399fb2 into kubernetes:master Jul 26, 2017
@foxish foxish deleted the foxish-api branch July 26, 2017 04:53
k8s-github-robot pushed a commit that referenced this pull request Aug 15, 2017
Merge pull request #49454 from dixudx/cleanup_DaemonSet_templateGener…
3537f8f 
…ation

Automatic merge from submit-queue

Cleanup DaemonSet templateGeneration

**What this PR does / why we need it**:

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #49336 

**Special notes for your reviewer**:

/cc @janetkuo @foxish 

~~Depends on #49071~~ (Merged) 

**Release note**:

```release-note
None
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dixudx dixudx dixudx left review comments

@caesarxuchao caesarxuchao caesarxuchao left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@liggitt liggitt liggitt left review comments

@janetkuo janetkuo janetkuo approved these changes

@0xmichalis 0xmichalis Awaiting requested review from 0xmichalis

Assignees

@smarterclayton smarterclayton

@janetkuo janetkuo

@bgrant0607 bgrant0607

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.