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 default tags to iam.Role #1000

Merged

Conversation

cebernardi
Copy link
Contributor

@cebernardi cebernardi commented Dec 10, 2021

Description of your changes

This PR adds the default Crossplane tags ("crossplane-kind", "crossplane-name", "crossplane-providerconfig") to IAMRoles

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • Unit-tests
  • Manual testing:
    • spin up a new kind cluster (kind create cluster --name crossplane-aws)
    • install CRDs (kubectl apply -f package/crds)
    • create resource scenario:
      • create an iam.Role in the cluster (kubectl apply -f examples/iam/role.yaml)
      • start crossplane-aws controller
      • verify that the tags "crossplane-kind", "crossplane-name", "crossplane-providerconfig" are added as expected
    • modify resource scenario:
      • delete tags from the resource: kubectl patch role.iam.aws.crossplane.io/somerole --type=json -p='[{"op": "remove", "path":"/spec/forProvider/tags"}]'
      • verify that the tags "crossplane-kind", "crossplane-name", "crossplane-providerconfig" are re-added as expected
    • start controller scenario:
      • stop provider-aws controller
      • reset the iam.Role (kubectl apply -f examples/iam/role.yaml)
      • start crossplane-aws controller
      • verify that the tags "crossplane-kind", "crossplane-name", "crossplane-providerconfig" are added as expected

@muvaf
Copy link
Member

muvaf commented Dec 11, 2021

Thanks @cebernardi ! #996 moves IAMRole in identity group to iam group with kind name Role and I expect that PR to land in the next few days. Can we hold this one and rebase once that's merged?

@cebernardi
Copy link
Contributor Author

yes sure!

@muvaf
Copy link
Member

muvaf commented Dec 13, 2021

@cebernardi It's merged, we should be good to rebase this now!

@cebernardi
Copy link
Contributor Author

nice, I'll rebase ASAP

@cebernardi cebernardi force-pushed the add-default-tags-to-iam branch from bae36d8 to 255a6f6 Compare December 15, 2021 11:58
@cebernardi cebernardi changed the title Add default tags to IAMRole Add default tags to iam.Role Dec 15, 2021
@cebernardi cebernardi force-pushed the add-default-tags-to-iam branch from 255a6f6 to 25a29f5 Compare December 15, 2021 12:40
@cebernardi cebernardi marked this pull request as ready for review December 15, 2021 12:42
@cebernardi
Copy link
Contributor Author

@muvaf I rebased and squashed

Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

Thank you @cebernardi and congrats on the 1000th PR!! It should be good to merge after fixing those small import issues.

pkg/controller/iam/role/controller_test.go Outdated Show resolved Hide resolved
pkg/controller/iam/role/controller_test.go Outdated Show resolved Hide resolved
@muvaf
Copy link
Member

muvaf commented Dec 15, 2021

@cebernardi we'll need you to sign DCO since it's your first contribution https://github.com/crossplane/provider-aws/pull/1000/checks?check_run_id=4535934088

@cebernardi cebernardi force-pushed the add-default-tags-to-iam branch from dba8bdc to c8c6c1b Compare December 15, 2021 17:39
@cebernardi
Copy link
Contributor Author

I should have incorporated your suggestions, @muvaf

Copy link
Member

@haarchri haarchri left a comment

Choose a reason for hiding this comment

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

LGTM - tested it - tagging is working - thanks ;)

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.

3 participants