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

dry-run: Create Options with dryRun for POST/PUT/PATCH #65105

Merged
merged 2 commits into from Jul 12, 2018

Conversation

@apelisse
Copy link
Member

apelisse commented Jun 14, 2018

What this PR does / why we need it:
Create new options for Create and Update (through POST/PUT/PATCH).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot requested review from krousey and liggitt Jun 14, 2018

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Jun 14, 2018

@apelisse apelisse force-pushed the apelisse:dry-run branch 4 times, most recently from 133048b to 45640e8 Jun 14, 2018

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/M labels Jun 18, 2018

@apelisse apelisse force-pushed the apelisse:dry-run branch from 45640e8 to c08181b Jun 18, 2018

@@ -65,7 +65,7 @@ func (s *storage) GetDeployment(ctx context.Context, deploymentID string, option
}

func (s *storage) CreateDeployment(ctx context.Context, deployment *extensions.Deployment, createValidation rest.ValidateObjectFunc) (*extensions.Deployment, error) {
obj, err := s.Create(ctx, deployment, createValidation, false)
obj, err := s.Create(ctx, deployment, createValidation, &metav1.CreateOptions{})

This comment has been minimized.

@lavalamp

lavalamp Jun 18, 2018

Member

Maybe make a metav1.CreateOptionsTODO() function so you can be sure to find and fix all of these after the fact? If you go this route, anyway.

@apelisse apelisse force-pushed the apelisse:dry-run branch 2 times, most recently from 9a4dd58 to cacf1ac Jun 19, 2018


// DryRun decides if the request should be completed or not.
// +optional
DryRun bool `json:"dryRun,omitempty" protobuf:"varint,1,opt,name=dryRun"`

This comment has been minimized.

@deads2k

deads2k Jun 21, 2018

Contributor

I think the decision from the call yesterday was to make this an enumerated string.

This comment has been minimized.

@apelisse

apelisse Jun 21, 2018

Author Member

Absolutely, I haven't taken the time to update yet.

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Jun 21, 2018

@sttts I'm behind at the moment. I've got a comment on the prereq pull and the API here needs an update. Got time while I deal with the downstream?


// If IncludeUninitialized is specified, the object may be
// returned without completing initialization.
IncludeUninitialized bool `json:"includeUninitialized,omitempty" protobuf:"varint,2,opt,name=includeUninitialized"`

This comment has been minimized.

@sttts

sttts Jun 22, 2018

Contributor

this surprises me. How is this related to dry-run? Why do we expose that via an API "suddenly"?

This comment has been minimized.

@sttts

sttts Jun 22, 2018

Contributor

Also note that initializers go away (if I have understood @smarterclayton correctly). We should not expose a parameter for something that is planned to be removed.

This comment has been minimized.

@sttts

sttts Jun 22, 2018

Contributor

Ok, found it: 69e757a#diff-7861f33474e4eb8d21cf1d33de5e4d4eL114 had the logic before, with a manually parsed get var. So nvm my comment above.

@@ -260,6 +260,14 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope RequestSco

ae := request.AuditEventFrom(ctx)
audit.LogRequestObject(ae, obj, scope.Resource, scope.Subresource, scope.Serializer)
} else {
if values := req.URL.Query(); len(values) > 0 {

This comment has been minimized.

@sttts

sttts Jun 22, 2018

Contributor

this is guarded by if checkBody. Looks odd.

This comment has been minimized.

@sttts

sttts Jun 22, 2018

Contributor

In the delete handler we call the guarding var allowsOptions.

This comment has been minimized.

@apelisse

apelisse Jun 25, 2018

Author Member

Yeah, I'm not sure what the point of these variable is, but it looks like "checkBody" and "allowOptions" have the same goal, so this is "correct".

@@ -111,7 +111,9 @@ func createHandler(r rest.NamedCreater, scope RequestScope, admit admission.Inte
}

// TODO: replace with content type negotiation?
includeUninitialized := req.URL.Query().Get("includeUninitialized") == "1"
options := &metav1.CreateOptions{

This comment has been minimized.

@sttts

sttts Jun 22, 2018

Contributor

Shouldn't this be using the parameter decoder? This looks hacky. Next commit.

}
}
// Timeout is not part of the options, drop it.
delete(values, "timeout")

This comment has been minimized.

@sttts

sttts Jun 22, 2018

Contributor

DecodeParameters ignores params that are not in the struct (necessary for new clients speaking to old servers). So, no need for the delete.

This comment has been minimized.

@apelisse

apelisse Jun 25, 2018

Author Member

Fixed

@@ -221,7 +221,8 @@ func (r *leaseEndpointReconciler) doReconcile(serviceName string, endpointPorts
}

glog.Warningf("Resetting endpoints for master service %q to %v", serviceName, masterIPs)
return r.endpointRegistry.UpdateEndpoints(ctx, e, rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc)
// PLEASE_PAY_ATTENTION(apelisse): No idea if this is what we need to do here. Does it make sense to have DryRun here anyway?

This comment has been minimized.

@sttts

sttts Jun 22, 2018

Contributor

this is the leader election code which speaks directly to the registry to update the (kubernetes service) endpoints. So this certainly does not need do a dry-run here. It is very very special purpose.

This comment has been minimized.

@apelisse

apelisse Jun 25, 2018

Author Member

Fixed

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Jul 10, 2018

forgot we didn't settle the param type... string vs []string

Heh, I just thought to ask about that too.

/approve cancel

@k8s-ci-robot k8s-ci-robot removed the approved label Jul 10, 2018

@apelisse apelisse force-pushed the apelisse:dry-run branch from 23a2b9a to aa7be5e Jul 11, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 11, 2018

@apelisse apelisse force-pushed the apelisse:dry-run branch from aa7be5e to b1ea57b Jul 11, 2018

@apelisse apelisse force-pushed the apelisse:dry-run branch 2 times, most recently from 765dc08 to a4b8a7e Jul 12, 2018

@@ -453,6 +453,42 @@ type DeleteOptions struct {
// foreground.
// +optional
PropagationPolicy *DeletionPropagation `json:"propagationPolicy,omitempty" protobuf:"varint,4,opt,name=propagationPolicy"`

// Whether the request should be completed and modifications will not be persisted.

This comment has been minimized.

@liggitt

liggitt Jul 12, 2018

Member

I'd suggest something more like this:

When present, indicates that modifications should not be persisted.
An invalid or unrecognized dryRun directive will result in a BadRequest response and no further processing of the request.

and then we can add details about what directives are supported as we determine that

@apelisse apelisse force-pushed the apelisse:dry-run branch from a4b8a7e to 4bfd5ec Jul 12, 2018

@apelisse

This comment has been minimized.

Copy link
Member Author

apelisse commented Jul 12, 2018

/retest

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Jul 12, 2018

type looks correct now.

/approve

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jul 12, 2018

/hold cancel
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Jul 12, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 12, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, deads2k, liggitt

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

@apelisse

This comment has been minimized.

Copy link
Member Author

apelisse commented Jul 12, 2018

/retest

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 12, 2018

Automatic merge from submit-queue (batch tested with PRs 65105, 62948). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit fe88461 into kubernetes:master Jul 12, 2018

17 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation apelisse authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-github-robot pushed a commit that referenced this pull request Jul 16, 2018

Kubernetes Submit Queue
Merge pull request #66041 from liggitt/manual-registries
Automatic merge from submit-queue (batch tested with PRs 66158, 66041, 66210). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove manually written typed registries

These were only used in a handful of places, and were not consistently available for all types.

They add a lot of call sites for PRs like #65105 and are not generally useful (very few callers have the ability to construct the underlying store).

This PR switches the scale subresources to use the underlying store directly (like the status subresources already were), and removes the manually written Registry impls.

/sig api-machinery
/kind cleanup
/assign @deads2k

/hold
will hold for #65105 and rebase after that

```release-note
NONE
```

k8s-publishing-bot added a commit to kubernetes/apiextensions-apiserver that referenced this pull request Jul 16, 2018

Merge pull request #66041 from liggitt/manual-registries
Automatic merge from submit-queue (batch tested with PRs 66158, 66041, 66210). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove manually written typed registries

These were only used in a handful of places, and were not consistently available for all types.

They add a lot of call sites for PRs like kubernetes/kubernetes#65105 and are not generally useful (very few callers have the ability to construct the underlying store).

This PR switches the scale subresources to use the underlying store directly (like the status subresources already were), and removes the manually written Registry impls.

/sig api-machinery
/kind cleanup
/assign @deads2k

/hold
will hold for kubernetes/kubernetes#65105 and rebase after that

```release-note
NONE
```

Kubernetes-commit: 43b801d499f95a4a89fe3319347225e9ad643359
@roycaihw

This comment has been minimized.

Copy link
Member

roycaihw commented Oct 1, 2018

It looks like we added meta.v1.CreateOptions and meta.v1.UpdateOptions in this PR, but they weren't reflected in our openapi spec. The dryRun query parameter was only added to the delete operations in the spec. I'm wondering if this was intended?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.