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

don't validate Min/Max block vals in CoerceValue #22478

Merged
merged 1 commit into from
Aug 16, 2019
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Aug 15, 2019

A provider may not have the data to fill in required block values in all
cases during the resource Read operation. This is more common in import,
because there is no initial configuration or state, and it's possible
some values are only provided in the configuration.

The original intent of MinItems and MaxItems in the schema was to
enforce configuration constraints, not to enforce what the resource
could save in the state. Since the configuration is already statically
validated, and the Schema is validated against the configuration in a
separate step, we can drop these extra validation constraints in
CoerceValue and relax it to only ensure the types conform to what is
expected.

Closes #22387
Issue 22387 is not an exhaustive list, but the representative that the issues are mostly encountered by the providers.

Also related to #22414, though that will require a change to the decoder spec as well. Unfortunately that does remove a layer of validation we would like to keep. We can fix that by creating a new function like AssertObjectCompatible that validates the final wholly-known value against the schema.

A provider may not have the data to fill in required block values in all
cases during the resource Read operation. This is more common in import,
because there is no initial configuration or state, and it's possible
some values are only provided in the configuration.

The original intent of MinItems and MaxItems in the schema was to
enforce configuration constraints, not to enforce what the resource
could save in the state.  Since the configuration is already statically
validated, and the Schema is validated against the configuration in a
separate step, we can drop these extra validation constraints in
CoerceValue and relax it to only ensure the types conform to what is
expected.
@jbardin jbardin requested a review from a team August 15, 2019 14:05
Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

coerceValue does a lot! From its stated documentation, I can see removing the min/max validation in coerce value, if that's not this function's job.

@jbardin jbardin merged commit 3e03213 into master Aug 16, 2019
@jbardin jbardin deleted the jbardin/coerce-value branch August 16, 2019 19:58
@paddycarver
Copy link
Contributor

@jbardin this is called from core, not the SDK, right? So providers don't need to re-vendor for this to be fixed?

@jbardin
Copy link
Member Author

jbardin commented Aug 16, 2019

No, this is only really called from the SDK. Though there will be a follow up PR that should be included with this, so there's no need to re-vendor it yet.

@paddycarver
Copy link
Contributor

👍 Thanks!

@ghost
Copy link

ghost commented Sep 16, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Sep 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple providers fail on refresh: Insufficient items for attribute
3 participants