-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(vertexai): make contents_delta_uri a required field in google_vertex_ai_index
#9066
fix(vertexai): make contents_delta_uri a required field in google_vertex_ai_index
#9066
Conversation
Hello! I am a robot. It looks like you are a: Community Contributor @c2thorn, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
@@ -100,6 +100,7 @@ properties: | |||
properties: | |||
- !ruby/object:Api::Type::String | |||
name: 'contentsDeltaUri' | |||
required: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change and can usually only be added within a major release (of which we have a very short window for this to get into)
Additionally, I cannot find it written that this field is required in all scenarios. The wording in https://cloud.google.com/vertex-ai/docs/vector-search/configuring-indexes makes it seem like it is not strictly required, where other required fields are explicitly labeled so. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change and can usually only be added within a major release
Hello @c2thorn, this change looks to correspond to the second example. So I believed we can release this in a minor release.
magic-modules/docs/content/develop/make-a-breaking-change.md
Lines 41 to 54 in aef0da0
## In minor releases | |
If a breaking change fixes a bug that impacts **all configurations** that | |
include a field or resource, it is generally allowed in a minor release. For | |
example: | |
* Removing update support from a field if that field is not actually updatable | |
in the API. | |
* Marking a field required if omitting the field always causes an API error. | |
The following types of changes can be made if the default behavior stays the | |
same and new behavior can be enabled with a flag: | |
* Major resource-level or field-level behavioural changes |
Additionally, I cannot find it written that this field is required in all scenarios.
That's true. I couldn't find any description either. I also want to know if it's optional or required. Can you reach out to the product team? Or should I reach out to the team by creating a support ticket or asking a technical account manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll reach out to the product team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's required on Create but not Update, which is maybe why the wording is not concrete. I think moving forward with marking it as required overall is the best choice.
Hello @c2thorn, this change looks to correspond to the second example. So I believed we can release this in a minor release.
Thanks for pointing this out, that looks to be correct. Let's move forward with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's required on Create but not Update
I see. Thank you for letting me know!
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 11 insertions(+), 11 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBigQueryDataTable_bigtable|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccFolderIamPolicy_basic |
Rerun these tests in REPLAYING mode to catch issues
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overriding breaking change lock due to this field causing an error if omitted.
Fixes hashicorp/terraform-provider-google#15962
Part of hashicorp/terraform-provider-google#12818
Add
required: true
tocontens_delta_url
to be able to avoid the following error at terraform apply:Release Note Template for Downstream PRs (will be copied)