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.

Sign up for GitHub

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

Jump to bottom

fix(vertexai): make contents_delta_uri a required field in google_vertex_ai_index #9066

Merged
merged 1 commit into from
Sep 25, 2023
Merged

fix(vertexai): make contents_delta_uri a required field in google_vertex_ai_index #9066

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mmv1/products/vertexai/Index.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ properties:
properties:
- !ruby/object:Api::Type::String
name: 'contentsDeltaUri'
required: true
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

@shotarok shotarok Sep 25, 2023 •
edited
Loading

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!

description: |-
Allows inserting, updating or deleting the contents of the Matching Engine Index.
The string must be a valid Cloud Storage directory path. If this
Expand Down