✨ Update .spec.config unmarshalling to accept yaml and json inputs #2266
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2266 +/- ##
==========================================
+ Coverage 73.00% 73.20% +0.19%
==========================================
Files 88 88
Lines 8743 8762 +19
==========================================
+ Hits 6383 6414 +31
+ Misses 1947 1938 -9
+ Partials 413 410 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
perdasilva
force-pushed
the
config-unmarshal
branch
from
38bbd2e to
b870100
Compare
| if err := yaml.Unmarshal(ext.Spec.Config.Inline.Raw, &cfg); err != nil { | ||
| return "", errors.New("inline config is not a valid JSON/YAML object") |
There was a problem hiding this comment.
IMHO, we cannot get an error here at parsing because k8s api-server reject invalid payload. Hence, it is enough to do:
_ = yaml.Unmarshal(ext.Spec.Config.Inline.Raw, &cfg);See similar approach at Flux HelmRelease resource.
There was a problem hiding this comment.
We can still get an error here because we aren't unmarshalling to a map[string]interface{}. But this does points to another issue with the error message: the input could still be valid JSON/YAML just not valid registry+v1 configuration.
There was a problem hiding this comment.
Another case where it could produce an error is if you do something like kubectl create --raw ... I think...but that's an edge case
There was a problem hiding this comment.
I'd also like to follow this PR up with some kind of ingress/validation tests. I'm wondering if those would be too heavy to be in the e2e suite (in the sense that the cost of execution is kinda of high - deploy cluster, etc.). But, I'm not sure if there's an easy way to exercise validation outside of the e2e suite. Any ideas?
There was a problem hiding this comment.
We can still get an error here because we aren't unmarshalling to a
map[string]interface{}. But this does points to another issue with the error message: the input could still be valid JSON/YAML just not valid registry+v1 configuration.
why do we unmarshall to a private type here? We should document this in the API, because it looks that inline field can be an arbitrary thing.
There was a problem hiding this comment.
it's arbitrary from the API side but it will be specific to the underlying bundle type:
- Helm -> values files and gated by values.json.schema
- registry+v1 -> its own internal configuration structure
- -> whatever it uses
There was a problem hiding this comment.
I've updated the API documentation to reflect this.
There was a problem hiding this comment.
I've updated the API documentation to reflect this.
Can you be more specific here, instead of stating
inline accepts arbitrary JSON/YAML objects whose validation is carried out at runtime against the schema provided by the bundle if one is provided.
so that we can help users to understand better what can be put there. Something along your previous comment:
Helm -> values files and gated by values.json.schema
registry+v1 -> its own internal configuration structure
with documenting how this registry+v1 structure looks like helps more.
Are we solving here a bug or there is in user facing experience? |
I had the same question. I originally had 🐛 but then because its not released made it ✨ - but I don't have a strong opinion. Happy to switch to 🐛 if this makes more sense. |
|
It would be nice we ensure good titles and the right emojis since that is the info that generates our release notes. So, whatever we add as PR title will be our release notes. |
No doubt. The question is which is the right emoji? |
|
If is a bug 🐛 |
perdasilva
force-pushed
the
config-unmarshal
branch
from
b870100 to
f63276e
Compare
@perdasilva Can you perhaps rephrase PR title and description to highlight the concrete improvement? |
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
perdasilva
force-pushed
the
config-unmarshal
branch
from
f63276e to
f7bf7a9
Compare
perdasilva
changed the title
perdasilva
changed the title
perdasilva
force-pushed
the
config-unmarshal
branch
from
f7bf7a9 to
d771437
Compare
perdasilva
force-pushed
the
config-unmarshal
branch
from
d771437 to
d2944a8
Compare
api/v1/clusterextension_types.go
Outdated
| // inline accepts arbitrary JSON/YAML objects whose validation is carried out at runtime against the schema provided | ||
| // by the bundle if one is provided. |
There was a problem hiding this comment.
Can we be more specific here, instead of stating
inline accepts arbitrary JSON/YAML objects whose validation is carried out at runtime against the schema provided by the bundle if one is provided.
so that we can help users to understand better what can be put there. Something along your previous comment:
Helm -> values files and gated by values.json.schema
registry+v1 -> its own internal configuration structure
with documenting how this registry+v1 structure looks like helps more.
There was a problem hiding this comment.
I kept those out of the documentation on purpose because I don't think we want to make the bundle format a user concern and we don't yet support the Helm format anyway.
Maybe there's a better way to communicate that the configuration schema is optionally provided by the bundle and the input is validated against that, but I'm struggling to find it. Maybe you could suggest changes and we could iterate together on that?
I don't think we should call out formats here though. The mental model is:
- bundles optionally provide a config schema
- users can set the configuration for the bundle through .spec.config
- the user input is validated at runtime against the configuration schema provided by the bundle
- if the bundle does not provide a configuration schema the process will continue on a best-effort basis (enforcing configuration schema is a catalog policy concern at this time)
Would even the list above be an improvement?
There was a problem hiding this comment.
Would even the list above be an improvement?
IMHO, yes.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: camilamacedo86, pedjak The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
perdasilva
force-pushed
the
config-unmarshal
branch
from
d2944a8 to
aefe385
Compare
|
New changes are detected. LGTM label has been removed. |
3 participants
Description
Changes
.spec.config.inline.Rawsupport both yaml and json input by switching tok8s.io/yamlto unmarshall the content.spec.configand.spec.config.inlinedoc strings to call out that it accepts arbitrary yaml/json objects that are validated at runtime by a schema provided by the underlying bundleMotivation
Ensure ClusterExtension .spec.config.inline can handle both JSON and YAML formats, which are supported by Kubernetes and that validation is carried out whether the input passed through client- and/or server-side validation before reaching the controller or not.
Additionally, to ensure that the api documentation highlights that configuration schema is defined by the bundle and the input is validated at runtime against that schema.
Reviewer Checklist