chore: add readOnlyProperties to metadata #1898
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1898 +/- ##
==========================================
+ Coverage 49.29% 49.36% +0.07%
==========================================
Files 46 47 +1
Lines 6804 6814 +10
==========================================
+ Hits 3354 3364 +10
Misses 3206 3206
Partials 244 244 ☔ View full report in Codecov by Sentry. |
| // - `Foo/Bar/Baz` => `foo/bar/baz` | ||
| // - `Foo/*/BarBaz` => `foo/*/barBaz` | ||
| func (r *ResourceProperty) ToSdkName() string { | ||
| arrayProps := strings.Split(string(*r), "/*/") |
There was a problem hiding this comment.
I think this would panic right now when called on an r that's nil.
There was a problem hiding this comment.
contract.Assertf(r != nil, "ResourceProperty should not be nil")
panics are good sometimes - this should not be nil I think.
There was a problem hiding this comment.
I'd argue that the provider shouldn't crash on a panic and rather exit with a proper error message.
If this shouldn't be nil, then we shouldn't accept a nil as the input imo. From what I can tell we're actually using it on a regular string and not a pointer right now
There was a problem hiding this comment.
That's a better idea. I checked around and this seems fine:
func (r ResourceProperty) ToSdkName() string
There was a problem hiding this comment.
If you want to be more pedantic, which is not always bad, then this:
n := ResourceProperty(strings.TrimPrefix(v.(string), "/properties/"))
Should instead be:
parseResourceProperty(string) (ResourceProperty, error)
There was a problem hiding this comment.
Switched it to not be a pointer receiver.
provider/pkg/metadata/metadata.go
Outdated
| @@ -22,6 +22,7 @@ type CloudAPIResource struct { | |||
| AutoNamingSpec *AutoNamingSpec `json:"autoNamingSpec,omitempty"` | |||
| Required []string `json:"required,omitempty"` | |||
| CreateOnly []string `json:"createOnly,omitempty"` | |||
| ReadOnly []string `json:"readOnly,omitempty"` | |||
There was a problem hiding this comment.
Here is an opportunity for a documentation comment on what this is about and a few examples of how this data looks like, specifically that nested names will be separated by "/", and that the names themselves are using Pulumi notation e.g. groupArn. If array element separator is used (is it '*')? also good to document there. It is still a little weird that "/" is the separator but that we can live with.
Having a doc comment will save future maintainers some jumping around! Thank you.
This PR adds `readOnlyProperties` to the pulumi metadata. It also fixes an issue with all properties (`readOnly/createOnly/writeOnly`) where nested properties where not converted to the correct sdk name value.
corymhall
force-pushed
the
corymhall/read-only-metadata
branch
from
b7c6613 to
17d3bad
Compare
This PR adds
readOnlyPropertiesto the pulumi metadata. It also fixesan issue with all properties (
readOnly/createOnly/writeOnly) wherenested properties where not converted to the correct sdk name value.