Skip to content

Add support for specifying wellKnownPolicies when using addons#4161

Merged
aclevername merged 7 commits intoeksctl-io:mainfrom
aclevername:well-known-addons
Sep 14, 2021
Merged

Add support for specifying wellKnownPolicies when using addons#4161
aclevername merged 7 commits intoeksctl-io:mainfrom
aclevername:well-known-addons

Conversation

@aclevername
Copy link
Contributor

Description

Closes #4158

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@aclevername aclevername added the kind/feature New feature or request label Sep 2, 2021
@aclevername aclevername marked this pull request as ready for review September 2, 2021 12:31
@aclevername aclevername requested review from Callisto13, Himangini, Skarlso and cPu1 and removed request for cPu1 September 2, 2021 13:15
@Callisto13 Callisto13 changed the title add support for specifying wellKnownPolicies when using addons Add support for specifying wellKnownPolicies when using addons Sep 13, 2021
resourceSet = builder.NewIAMRoleResourceSetWithAttachPolicy(addon.Name, namespace, serviceAccount, addon.PermissionsBoundary, addon.AttachPolicy, a.oidcManager)
}
return resourceSet.OutputRole, nil
return resourceSet, resourceSet.AddAllResources()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is so weird. It returns the resource, but then also returns the output of calling AddAllResources on the created resource. The cognitive load of this "simple" function is pretty high. I would prefer this to be separated and the caller has the responsibility to further add all resources if necessary. The createroleResourceSet doesn't suggest that this will also add all of them. It suggest construction.

Copy link
Contributor Author

@aclevername aclevername Sep 14, 2021

Choose a reason for hiding this comment

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

I think whats weird is how the resourceSets work. You use a constructor to "build" it, but its only actually build it/becomes useful after you've called AddAllResources. I could move this call further up, but its just moving the error handling around even more 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, well that's annoying. :D Fine, leave it as is :)

Copy link
Contributor

@Skarlso Skarlso left a comment

Choose a reason for hiding this comment

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

One question / note. Otherwise the generated function and logic look okay to me. I'm not sure though if anything might be missing from somewhere. In my limited understanding of the current code structure and focal points of eksctl this looks okay. :)

@aclevername aclevername merged commit a1c63c2 into eksctl-io:main Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for wellKnownPolicies when defining addons

3 participants