Introduce spec.free to catalog restrictions for Plans
#2211
Conversation
k8s-ci-robot
added
cncf-cla: yes
jeremyrickard
changed the title
spec.free to catalog restrictions for Plansspec.free to catalog restrictions for Plans
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
Here is the docs preview: https://deploy-preview-2211--svc-cat.netlify.com/docs/catalog-restrictions/ |
jeremyrickard
changed the title
spec.free to catalog restrictions for Plansspec.free to catalog restrictions for Plans
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
docs/catalog-restrictions.md
Outdated
| The rule format is expected to be `<property><conditional><requirement>` | ||
|
|
||
| * `<conditional>` is allowed to be one of the following: ==, !=, in, notin | ||
| * `<requirement>` will be a string value if `==` or `!=` are used. |
There was a problem hiding this comment.
suggestion: combine into a single bullet will be a string value if.... otherwise it will be a set of string values if ...
There was a problem hiding this comment.
Let's also add a bullet for the property and give them a hint or a few examples of what it should look like, i.e. it should have the leading spec. prefix
docs/catalog-restrictions.md
Outdated
| | Property Name | Description | | ||
| | name | the value set to ClusterServiceClass.Name | | ||
| | spec.externalName | the value set to ClusterServiceClass.Spec.ExternalName | | ||
| | spec.externalID | the value set to ClusterServiceClass.Spec.ExternalID | |
There was a problem hiding this comment.
"the value set to " is a bit confusing. Can we use a plain english description or maybe something else? I'm not quite sure what to suggest, sorry!
docs/catalog-restrictions.md
Outdated
|
|
||
| This example creates a Service Class restriction on spec.externalName using the | ||
| `in` operator. In this case, only services that have the externalName | ||
| `FooService` or `BarService` will have Service Catalog resources created. |
There was a problem hiding this comment.
Is the filter case sensitive? i.e. would "fooservice, barservice" work too?
There was a problem hiding this comment.
that's a good question! let me read the code and figure that out. Will add relevant text to this to make sure!
| toc: | ||
| - docs/catalog-restrictions.md | ||
| - title: Catalog Restrictions | ||
| path: /docs/catalog-restrictions/#catalog-restrictions |
There was a problem hiding this comment.
Just curious if you have tried making these paths just path: #catalog-restrinctions and if that works or not? I'm starting to wonder if I imagined it. 😁
docs/catalog-restrictions.md
Outdated
| In this example, a catalog restriction has been defined that specifies that | ||
| only service plans that have an external name of basic should be selected. | ||
| Catalog restrictions are defined as a set of one or more rules that target | ||
| either service classes and service plans. These rules have a special format |
There was a problem hiding this comment.
Catalog restrictions are defined as a set of one or more rules that target either service classes and service plans
Because of the word either, I think and should be an or. Perhaps we should say "that target service classes and/or service plans"
There was a problem hiding this comment.
Ahh yeah thanks! Forgot to remove that word. You can supply both. so and/or
| | spec.externalID | This key will match the ServicePlan.Spec.ExternalID property | | ||
| | spec.free | This key will match the ServicePlan.Spec.Free property | | ||
| | spec.serviceClass.name | This key will match the ServicePlan.Spec.ServiceClassRef.Name property | | ||
|
|
There was a problem hiding this comment.
Since the property key and descriptions are the same for the NS and Cluster scoped Plan/Class, I'd suggest combining them. IE "ClusterServicePlan and ServicePlan allowed property names"
Same with the Classes
There was a problem hiding this comment.
They are almost the same. There are some shared keys, but we also allow to supplies predicates that reference the non-common parts, i.e. ServicePlans can be filtered with ServiceClassRef.Name and ClusterServicePlans can be filtered with ClusterServiceClassRef.Name.
Would you favor doing a shared block and then call out the two differences?
There was a problem hiding this comment.
@jeremyrickard I missed that. I totally agree, leave it as is, thank you.
jeremyrickard
force-pushed
the
catalog-restrictions-free-plans
branch
from
d95bfc9 to
79b7e9d
Compare
|
@carolynvs @jboyd01 I've updated this PR to add this doc to the README.md under topics for operators and I modified the namespaced doc to link here, as was suggested in that PR. Take a 👀 |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carolynvs 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 |
k8s-ci-robot
added
the
approved
This PR extends the existing catalog restrictions to allow the user to specify
spec.freewhen providing [Cluster]ServicePlan catalog restrictions. This also adds relevant documentation for catalog restrictions.Closes: #2209, #2156