-
Notifications
You must be signed in to change notification settings - Fork 74
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(tags): import of tag resources failed with 403 error #620
base: main
Are you sure you want to change the base?
Conversation
ad48e56
to
a8fe9e3
Compare
Signed-off-by: Nicolas Levée <[email protected]>
a8fe9e3
to
4170db9
Compare
Hi! Is there something wrong ? Please can you check if I have to rework something 🙏 |
Hi @ulucinar @sergenyalcin @turkenf, Is there anything I can do to pass this PR ? |
@nlevee We're aiming for a new provider release next week and will look to get this PR in for it. |
/test-examples="examples/tags/v1beta1/tagkey.yaml" |
/test-examples="examples/tags/v1beta1/tagvalue.yaml" |
/test-examples="examples/tags/v1beta1/tagbinding.yaml" |
Done 👍 thank you |
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.
Hi @nlevee, thank you for your efforts in this PR. I left two comments for your consideration as an initial review.
@@ -19,15 +19,18 @@ func Configure(p *config.Provider) { | |||
TerraformName: "google_tags_tag_value", | |||
Extractor: common.ExtractResourceIDFuncPath, | |||
} | |||
config.MarkAsRequired(r.TerraformResource, "parent", "tag_value") |
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 do we mark these fields as required? and for the other two resources?
apiVersion: tags.gcp.upbound.io/v1beta1 | ||
kind: TagBinding | ||
metadata: | ||
annotations: | ||
meta.upbound.io/example-id: tags/v1beta1/tagbinding | ||
upjet.upbound.io/manual-intervention: "The resource requires a real external name" | ||
crossplane.io/external-name: "&{tagBindingID}" | ||
labels: | ||
testing.upbound.io/example-name: binding | ||
name: binding-external | ||
spec: | ||
managementPolicies: ["Observe"] | ||
forProvider: {} | ||
|
||
--- |
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.
We do not prefer to have examples of the same kind in the same YAML file. If the example you added is for a different scenario, for example, observe, it would be better to add a new example named tagbinding-observe.yaml
.
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.
@nlevee, I have successfully tested the import of an existing resource with the example you gave, but when I try to create the resources from scratch, I see the following error. Can you create the resources successfully?
conditions:
- lastTransitionTime: "2024-11-01T15:05:53Z"
message: 'observe failed: failed to observe the resource: [{0 Error when reading
or editing TagsTagKey "key": googleapi: Error 400: com.google.apps.framework.request.StatusException:
<eye3 title=''INVALID_ARGUMENT''/> generic::INVALID_ARGUMENT: Invalid CRM resource
name: ''tagKeys/key''. []}]'
reason: ReconcileError
status: "False"
type: Synced
Description of your changes
I've changed the way tags are imported when “managementPolicies” is set to “Observe”.
Currently, the import fails with at best a 403 error.
The problem is that the Terraform provider's import function expects to receive the “name” as an argument, with only the name as the value (without the “tagXXX/” prefix).
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Tested with resources in example directory.
Link to #627