Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Define default provisioning params on plans #2282

Conversation

carolynvs
Copy link
Contributor

@carolynvs carolynvs commented Aug 15, 2018

This allows an operator to define a plan and provide a default set of provisioning parameters. I am doing this incrementally so that it's not a giant PR. There will be follow-up PRs to add: svcat create plan --from, defaults on classes, etc. The set of milestones is laid out in the proposal.

TODO:

  • Add feature gate
  • Documentation

Doc preview available at https://deploy-preview-2282--svc-cat.netlify.com/docs/service-plan-defaults/

@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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 15, 2018
@carolynvs carolynvs force-pushed the default-plan-provisioning-params branch from c9cf6ee to fc1a0f3 Compare August 20, 2018 16:53
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2018
* Vendor mergemap which helps merging parameters from multiple sources
@carolynvs carolynvs force-pushed the default-plan-provisioning-params branch from 11e4fcb to 52e9628 Compare September 12, 2018 16:17
@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 Sep 12, 2018
@carolynvs carolynvs changed the title WIP: Define default provisioning params on plans Define default provisioning params on plans Sep 12, 2018
@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 12, 2018
@carolynvs carolynvs requested review from jeremyrickard and removed request for jberkhahn September 12, 2018 16:18
@carolynvs carolynvs force-pushed the default-plan-provisioning-params branch 2 times, most recently from 06116fc to 131ef41 Compare September 12, 2018 16:24
After a service instance resolves a class and plan, reconcile the
default parameters defined on the plan with those from the instance.

Persist the original default parameters on the instance status
so that they are not applied more than once, and also help
with debugging.
We need to be able to update the spec on both instances and bindings eventually so that
default parameters can be applied for both provisioning and binding.
@carolynvs carolynvs force-pushed the default-plan-provisioning-params branch from 21c589a to a24fd45 Compare September 12, 2018 17:49
Plan: custom-mysql

Parameters:
No parameters defined
Copy link
Contributor

Choose a reason for hiding this comment

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

this was confusing a bit at first glance. I guess this is just the stuff provided from the svcat command the user entered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, it is whatever they first entered in. I agree it's confusing and would like to do a follow-up PR for the svcat UX. This is a pretty big PR as-is, so I tried to limit it to just the server side changes and documentation.

Copy link
Contributor

@jboyd01 jboyd01 left a comment

Choose a reason for hiding this comment

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

This looks solid to me.
/approve

@@ -1184,6 +1189,7 @@
"pkg/util"
]
revision = "6702109cc68eb6fe6350b83e14407c8d7309fd1a"
version = "kubernetes-1.11.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some changes in here I wasn't expecting, but I guess that is just due to a newer point release of Dep you are using? ie there are some differences in version and branch elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has to do with the information available in git at the time we last edited the lock file. When someone updated us to 1.11, they pointed at the branch, not the tag I'm assuming.

This won't change the commit used in our vendor directory, just tweaks the breadcrumb trail on how to find that SHA (i.e. prefer the tag over a moving branch).

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jboyd01

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 Sep 13, 2018
@jeremyrickard
Copy link
Contributor

I gave this a spin on my AKS cluster and it looks good!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2018
@k8s-ci-robot k8s-ci-robot merged commit b667117 into kubernetes-retired:master Sep 14, 2018
@carolynvs carolynvs deleted the default-plan-provisioning-params branch September 14, 2018 17:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants