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

Add tags management to OpenIDConnectProvider (iam) #1059

Merged
merged 1 commit into from Feb 28, 2022
Merged

Add tags management to OpenIDConnectProvider (iam) #1059

merged 1 commit into from Feb 28, 2022

Conversation

cebernardi
Copy link
Contributor

@cebernardi cebernardi commented Jan 10, 2022 •
edited

Description of your changes

Added tags management to OpenIDConnectProvider.

Fixes #

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

This PR was tested by:

  • Unit Tests
  • The following manual testing was performed in a local kind cluster configured with an AWS account:
    • OIDCProvider creation: the provider was correctly created and the default tags were added
    • OICDProvider update:
      • New tags were added to the CR and they were successfully created in the external AWS resource
      • Tags values were modified in the CR, and they were successfully modified in the external AWS resource
      • Tags were removed from the CR and they were successfully removed from the external AWS resource
      • The default tags were removed from the CR, and they were restored in the CR and NOT removed from the external AWS resource

@cebernardi
Copy link
Contributor Author

I'm opening a draft PR because I added a testing structure to explicitly test the aws input structs involved in the tag and untag operations, and for best readability, I split the tests for the Update method (related to specifically update tags) in a separate test function, and I would like to have some feedback on this before finalizing the PR.

In particular, if the proposed testing approach is accepted, I'm keen to introduce the same to the others operations on the other attributes (thumbprint, ClientID), maybe in separate PRs to help the review?

@cebernardi
Copy link
Contributor Author

@muvaf FYI

@cebernardi cebernardi changed the title Iam oicdprovider add tags Add tags management to OpenIDConnectProvider (iam) Jan 11, 2022
@haarchri haarchri requested a review from muvaf January 11, 2022 15:19
@cebernardi cebernardi force-pushed the iam-oicdprovider-add-tags branch 2 times, most recently from 1f97c9d to 2f81cd8 Compare January 19, 2022 17:28
@cebernardi cebernardi marked this pull request as ready for review January 19, 2022 17:28
@haarchri haarchri mentioned this pull request Feb 9, 2022
Closed
2 tasks
@cdenneen
Copy link
Contributor

cdenneen commented Feb 9, 2022

Fixes: #1133

cdenneen
cdenneen approved these changes Feb 9, 2022
View reviewed changes
Copy link
Contributor

@cdenneen cdenneen left a comment

Choose a reason for hiding this comment

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

lgtm

muvaf
muvaf approved these changes Feb 18, 2022
View reviewed changes
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.

Thanks @cebernardi ! Left a couple of comments but should be good to merge once they are addressed. Let me know when it's ready to go!

pkg/clients/iam/openidconnectprovider_test.go Outdated Show resolved Hide resolved
pkg/clients/iam/tags.go
// to current and desired states;
// tags that have changed will be returned in the addOrUpdate return parameter, but not included in the `remove` return parameters
// it also returns if desired state needs to be updated
func DiffIAMTagsWithUpdates(local []v1beta1.Tag, remote []iamtypes.Tag) (addOrUpdate []iamtypes.Tag, remove []string, areTagsUpToDate bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use this for all cases? Do other API endpoints in IAM return error when you call Tag for an existing tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think we should always use this semantic, as in all the AWS docs I've read, if you call Tag with an existing key but a different value, it overrides the value (as expected, I would say).

But all the other resources are using a remove tag / add tag semantic, and I wanted to keep the scope of this PR to the bare minimum.

I'll be happy to address it on a separate PR?

pkg/controller/iam/openidconnectprovider/controller.go Outdated Show resolved Hide resolved
pkg/controller/iam/openidconnectprovider/controller.go Outdated Show resolved Hide resolved
pkg/controller/iam/openidconnectprovider/controller_test.go
@@ -20,6 +20,11 @@ import (
"context"
"testing"

"github.com/google/go-cmp/cmp/cmpopts"
Copy link
Member

Choose a reason for hiding this comment

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

Import grouping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave it a try, hoping I got it right...

apis/iam/v1beta1/openidconnectprovider_types.go Outdated Show resolved Hide resolved
@cebernardi
added tags management to OIDCProvider
7dad1bb 
Signed-off-by: Cecilia Bernardi <cbernardi@expediagroup.com>
@muvaf muvaf merged commit 9586cfc into crossplane-contrib:master Feb 28, 2022
@haarchri haarchri mentioned this pull request Apr 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdenneen cdenneen cdenneen approved these changes

@muvaf muvaf muvaf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cebernardi @cdenneen @muvaf