-
Notifications
You must be signed in to change notification settings - Fork 211
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 skeleton test for Vertex AI resources #2244
Fix skeleton test for Vertex AI resources #2244
Conversation
@maqiuyujoyce Could you help take a look if this PR makes sense? |
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.
LGTM
@@ -64,7 +64,7 @@ spec: | |||
- name: google_vertex_ai_endpoint | |||
kind: VertexAIEndpoint | |||
idTemplate: "projects/{{project}}/locations/{{region}}/endpoints/{{name}}" | |||
idTemplateCanBeUsedToMatchResourceName: false | |||
idTemplateCanBeUsedToMatchResourceName: 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.
Is this change on purpose? This property/field is usually set to true when we want to enable import/export tool for this resource. Or is it configured because we want to use export for vertex AI resources in e2e tests?
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 am not familiar with this field. But according to #2135, if this field is set to false
, test TestNewFromURI
will skip on the resource?
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.
You are right. And the field was designed for the export/import tool: https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/pkg/apis/core/v1alpha1/servicemapping_types.go#L88
ResourceConfigId: VertexAIDataset | ||
URI: "https://us-central1-aiplatform.googleapis.com/v1beta1/projects/kcc-test/locations/us-central1/datasets/1234567890?alt=json" | ||
URI: "" |
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.
Looking at other examples, URI may or may not be kept when ExpectedSkeleton is null. Do you mind adding a unit test to ensure the behavior is consistent? I think either not setting it (if possible) or setting it to an empty string should work.
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.
Related to the previous comment, the test TestNewFromURI
is currently skipped for VertexAIDataset
. Do we still need URI?
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 don't think we need 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.
Just double check on my understanding. I saw that resource VertexAIDataset
does not support import. Therefore we should set idTemplateCanBeUsedToMatchResourceName
to false
for this resource?
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.
In most cases, you are correct. More specifically, the decision flow I have for this field is:
- Do I need to support export for this resource? Yes: Go to step 2. No: Set the value to
false
. - Can the
IDTemplate
be used to match the resource's URL? Yes: Set the value totrue
. No: Go to step 3. - Figure out the export use case / design for this resource.
Whether TF support import of this resource or not can be helpful to answer the question in step 2 above.
ResourceConfigId: VertexAIEndpoint | ||
URI: "https://us-central1-aiplatform.googleapis.com/v1beta1/projects/kcc-test/locations/us-central1/endpoints/vertexaiendpoint-test" | ||
- ExpectedSkeleton: | ||
apiVersion: vertexai.cnrm.cloud.google.com/v1beta1 | ||
kind: VertexAIIndex | ||
metadata: | ||
name: vertexaiindex-test | ||
metadata: {} |
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.
Why is the metadata section cleared up?
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.
As the result of enabling the test TestNewFromURI
for resource (by setting idTemplateCanBeUsedToMatchResourceName
to true
), this is the expected output of the test. It is an output only field in API. So it ended up showing up under status.observedState.name
. Not sure if I miss configured the service mapping?
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.
Ah, then this is actually a good catch!
Looking at
metadata: {} |
However, it seems to me that the export won't work out-of-box because it can't generate a valid YAML manifest for this resource. (Context about import/export tool: https://cloud.google.com/config-connector/docs/how-to/import-export/export) This might indicate that idTemplateCanBeUsedToMatchResourceName: false
is the correct value for VertexAIIndex.
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.
Thanks! I set idTemplateCanBeUsedToMatchResourceName: false
for VertexAIIndex.
a591ff0
to
c4539cc
Compare
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maqiuyujoyce The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
120cc63
into
GoogleCloudPlatform:master
Change description
Fixes #
Tests you have done
make ready-pr
to ensure this PR is ready for review.