Skip to content

Conversation

Taragolis
Copy link
Contributor

@Taragolis Taragolis commented Jul 23, 2022

Separate class which help resolve and validate AWS connection parameters.
Currently, might for historical reason, connections properties validate/read/resolve/construct in BaseSessionFactory and AwsGenericHook in different methods.

This changes would help to make changes/deprecate in AWS Connection settings easier than now - need to make changes in one place, rather tried to found in multiple places.

Also it should make easier tests Connections configuration relates.

WDYT? @o-nikolas @ferruzzi

Note: BaseSessionFactory still use connection configs for SAML and Web Federation, I have to plan remove this in the further PRs

@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Jul 23, 2022
@Taragolis Taragolis force-pushed the aws-connection-wrapper branch 3 times, most recently from 6900359 to dda3fe7 Compare July 25, 2022 08:31
@Taragolis Taragolis force-pushed the aws-connection-wrapper branch from dda3fe7 to 79279cf Compare July 25, 2022 11:23
Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

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

LGTM. I love the cleaning! Thanks for a lot for doing it!

Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

Looks good, left a question below. Thanks for taking on so much of the cleanup work, it's a huge help.

[EDIT] My question is resolved, LGTM.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Way. Way cleaner. Thanks @Taragolis !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants