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.

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

add crossplane-external-name tag to external resources #409

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LCaparelli
Copy link

@LCaparelli LCaparelli commented May 24, 2024

Description of your changes

Ensures a new tag containing the value set to the external-name annotation exists in external resources.

Fixes #408

I kinda didn't wait for discussion to pan out on the issue, and I understand if this is not at all the direction we want to take. Just thought it'd be nice to present a problem along with a possible solution. If you have concerns about the direction, please feel free to chime in at the issue or ping me directly on Crossplane's Slack.

I have:

  • Read and followed Upjet's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

I have pulled provider-aws-upjet locally, checked out at the v1.5.0 tag, pointed the dependency to this branch using go.mod replace, built the EC2 provider and deployed it in our development environments. All EC2 managed resources now have this additional tag.

Signed-off-by: Lucas Caparelli <lucas.caparelli@gympass.com>
@LCaparelli
Copy link
Author

In order to make my fork work, I had to manually assign RBAC permissions to the generated ServiceAccount:

rules:
- apiGroups:
  - aws.upbound.io
  resources:
  - providerconfigs
  - providerconfigs/status
  - providerconfigusages
  - providerconfigusages/status
  - storeconfigs
  - storeconfigs/status
  verbs:
  - '*'

I think it might be because I only built the EC2 provider, and not the entire family. Honestly I'm not sure. I don't see how this change could interfere with that, so I'm guessing this is unrelated, but appreciate any pointers.

const externalResourceTagKeyExternalName = "crossplane-external-name"

func getExternalTags(mg xpresource.Managed) map[string]string {
externalTags := xpresource.GetExternalTags(mg)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the only call to xpresource.GetExternalTags I could find in the code, so I'm assuming this is the best and only place to provide this additional tag. If there are other places I'm missing, please let me know and I'll adjust.

@ulucinar
Copy link
Collaborator

ulucinar commented Jun 3, 2024

Hi @LCaparelli,
Would you like to discuss this proposal in the next sig-upjet meeting, which is scheduled for Wednesday, June 5 at 3:00pm GMT?

@LCaparelli
Copy link
Author

@ulucinar thanks, I would love to, but at that time I'll be on a holiday trip. Maybe o the next one?

I'm able to discuss async at #408 until then.

@jbw976
Copy link
Member

jbw976 commented Jun 3, 2024

@ulucinar feel free to checkout the recording of @LCaparelli presenting on this topic: #408 (comment)

@LCaparelli
Copy link
Author

@ulucinar I added/suggested a discussion topic to SIG Upjet's meeting backlog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure a tag containing external-name is present in all external resources supporting tags
3 participants