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

Provider to ProviderConfig #320

Merged
merged 34 commits into from
Aug 31, 2020
Merged

Provider to ProviderConfig #320

merged 34 commits into from
Aug 31, 2020

Conversation

muvaf
Copy link
Member

@muvaf muvaf commented Aug 24, 2020 •
edited

Description of your changes

Part of crossplane/crossplane#1684

Fixes #38 too

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation, examples, or release notes.
  • Updated the dependencies in app.yaml to include any new role permissions.

@muvaf
provider: add ProviderConfig type and deprecated old Provider type
ce287e8 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
Update all managed resources for pointer type ProviderConfigRef and P…
032452a 
…roviderRef

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
Bunch of managed resources are updated to use new Provider and Provid…
168b0b1 
…erConfig

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
internetgateway: update to new provider and providerconfig
df7c68c 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
routetable: update to new provider and providerconfig
fb94baa 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
securitygroup: update to new provider and providerconfig
aae2790 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
subnet: update to new provider and providerconfig
7b428bc 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
vpc: update to new provider and providerconfig
5ca8fd8 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
eks: update to new provider and providerconfig
3e6f6d1 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
nodegroup: update to new provider and providerconfig
73c0ef3 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
elb: update to new provider and providerconfig
dac09d9 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
elbattachment: update to new provider and providerconfig
09d647d 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
iamgroup: update to new provider and providerconfig
1035c7a 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
Copy link
Member Author

muvaf commented Aug 24, 2020

After all resources are migrated, I will need to add region fields to the spec of the ones that need region for their operations.

@muvaf
iamgrouppolicyattachment: update to new provider and providerconfig
b644ce9 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
iamgroupusermembership: update to new provider and providerconfig
8ca6d72 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
iampolicy: update to new provider and providerconfig
5132eec 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
iamrole: update to new provider and providerconfig
44e9bd7 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
iamrolepolicyattachment and iamuser: update to new provider and provi…
27a7cc7 
…derconfig

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
iamuserpolicyattachment: update to new provider and providerconfig
441d3df 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
snssubscription: update to new provider and providerconfig
393e6ee 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
snstopic: update to new provider and providerconfig
22a182e 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
redshift: update to new provider and providerconfig
e34dda9 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
hostedzone: update to new provider and providerconfig
e8a045f 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
resourcerecordset: update to new provider and providerconfig
ba91361 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
bucket: update to new provider and providerconfig
b12c2b0 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
bucketpolicy: update to new provider and providerconfig
9afea0f 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
make reviewable passes now
28d0382 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
utils folder is removed as it is not necessary anymore
ae6c599 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
Copy link
Member Author

muvaf commented Aug 24, 2020

All resources now use the same GetConfig method which takes care of backward compatibility and all quirks of AWS Authentication. Next step is to figure out which resources need region field. Right now, nothing is broken but if you use ProviderConfig, there is no field to specify region.

@muvaf
update go.mod with latest crossplane-runtime and crossplane-tools tha…
687f210 
…t has the necessary changes

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
register ProviderConfig during init
09d96ae 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf muvaf marked this pull request as ready for review August 27, 2020 16:08
@muvaf
Copy link
Member Author

muvaf commented Aug 27, 2020

This should be ready to go. @negz 's sentiment was right - region has always been just a default used during *aws.Config object creation, all resources already have zone/region fields if there needs to be.

@muvaf muvaf requested review from hasheddan and negz August 27, 2020 16:10
@muvaf
remove TypeMeta from DeepCopyIntoPC function since it is necessary
a11aefe 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf
use awscommon.GetConfig directly without func indirection
8e7bca0 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
hasheddan
hasheddan reviewed Aug 28, 2020
View reviewed changes
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

@muvaf This looks pretty good to me, main question is just on the apiVersion we want to use for ProviderConfig. Thanks for working on this tedious update :)

apis/v1alpha3/providerconfig_types.go Outdated
limitations under the License.
*/

package v1alpha3
Copy link
Member

Choose a reason for hiding this comment

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

Should we introduce ProviderConfig at v1alpha1?

Copy link
Member

Choose a reason for hiding this comment

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

I think there might even be a case for introducing it at v1beta1, given that it's an evolution of the Provider type which has been stable for some time.

Copy link
Member

Choose a reason for hiding this comment

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

That seems pretty reasonable to me

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. The fields are pretty lean already (I don't think we'll ever get rid of cred secret ref), so, if we want to add new functionality we can just expand this without breaking anyone.

@muvaf
update all examples to use spec.providerConfigRef instead of spec.pro…
b3c4966 
…viderRef

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf muvaf force-pushed the providemeconfig branch 2 times, most recently from 07cbb09 to 2ec4492 Compare August 31, 2020 12:11
@muvaf
providerconfig: bump to v1beta1
3863f0a 
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
hasheddan
hasheddan approved these changes Aug 31, 2020
View reviewed changes
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

Looks like CI got stuck so I just restarted, but LGTM! Thanks @muvaf 🎉

@muvaf
Copy link
Member Author

muvaf commented Aug 31, 2020

FYI, codecov fails because I removed all Connect tests because all they do differently is to call a different func so it's unnecessary to maintain tests of each one of them separately.

@muvaf muvaf merged commit da1a197 into crossplane-contrib:master Aug 31, 2020
@muvaf muvaf deleted the providemeconfig branch August 31, 2020 15:32
This was referenced Sep 1, 2020
Merged
Closed
@muvaf muvaf mentioned this pull request Oct 1, 2020
Closed
@kasey kasey mentioned this pull request Oct 1, 2020
Merged
4 tasks
wolffbe pushed a commit to wolffbe/provider-aws that referenced this pull request Feb 12, 2021
@muvaf
Merge pull request crossplane-contrib#320 from muvaf/providemeconfig
591661b 
Provider to ProviderConfig
namku pushed a commit to namku/provider-aws that referenced this pull request Mar 9, 2021
@muvaf
Merge pull request crossplane-contrib#320 from muvaf/providemeconfig
4e5de5e 
Provider to ProviderConfig
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasheddan hasheddan hasheddan approved these changes

@negz negz Awaiting requested review from negz

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

Successfully merging this pull request may close these issues.

The AWS cloud providers should not include the region
3 participants
@muvaf @negz @hasheddan