Add a new configuration option for required field generation #381
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sergenyalcin
requested review from
ulucinar,
erhancagirici and
mergenci
as code owners
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
sergenyalcin
force-pushed
the
add-required-configuration-option
branch
from
811aab6
to
bdfbe67
Compare
ulucinar
reviewed
Mar 18, 2024
There was a problem hiding this comment.
Thanks @sergenyalcin, left some comments for you to consider.
…iredFields - Deprecate config.MarkAsRequired in favor of a new configuration function on *config.Resource that still accepts a slice to mark multiple fields as required without doing and invervention in native field schema. Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
sergenyalcin
force-pushed
the
add-required-configuration-option
branch
from
fb16c91
to
f25329f
Compare
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
ulucinar
approved these changes
Mar 19, 2024
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
3 tasks
|
The tests in GCP are successful: crossplane-contrib/provider-upjet-gcp#490 (comment) |
|
Successfully created backport PR #383 for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of your changes
This PR adds a new configuration option for required field generation. The name of this new configuration API is
RequiredFieldsand it is included to theResource.This change aims to generate fields as
Requiredwhen needed, without interfering with the underlying provider schema. Before, for generating a field asRequired, we used the MarkAsRequired function. This function directly intervenes in the schema and sets theComputedandOptionalvalues of the relevant field equal tofalse. Schema interventions may have the power to directly affect the internal operations of the underlying provider. This may lead to unexpected behavioral changes. We recently observed the mentioned situation in this issue.In this issue, it was observed that the behavior of resources that schema was intervened (using
MarkAsRequired) changed after the underlying provider version bump, directly related to this schema intervention. This behavioral change directly blocks the provisioning of resources and even causes existing resources beunsynced. This reveals the extent of the dangers of schema interventions.Of course, there are such needs for generation. However, such needs can be solved without schema intervention. This change paves the way for a field that is not required in the underlying provider schema to be generated as 'Required', with a high-level configuration API, without any schema intervention.
This PR also proposes the deprecation of
MarkAsRequiredAPI.I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Tested generation in provider-gcp by removing MarkAsRequired and adding the following configuration: