baolsen commented on issue #7619: [AIRFLOW-6975] Base AWSHook AssumeRoleWithSAML URL: https://github.com/apache/airflow/pull/7619#issuecomment-598012580 > The `_assume_role_with_saml` method looks a bit _complex_ to me, because you are importing so many different libraries and you are doing a lot of semantic blocks which are showing that it could be useful to split it - So I would probably refactor it. > > In my opinion the whole `_get_credentials` is too _complex_. Maybe it is time to add a sub-class `AwsCredentialsExtractor` or similar where we can split this whole function into smaller pieces we also fully test. > > But in general it really looks good to me. 👍 You added the documentation to the how-to which is great :) It is getting quite complex. I think I'd rather leave it for now, until the AWS system tests are in place and then refactoring and unit testing will be a possibility :)
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
