Skip to content

Conversation

@dominikhei
Copy link
Contributor


As discussed on #51196 there are some more operators, that set the aws_conn_id in their constructor, even though it is set by the base operator from which they inherit. This does not lead to any errors, but is not consistent, which is why I removed it. Feel free to close the PR if you think it's unnecessary.

@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels May 30, 2025
@dominikhei dominikhei marked this pull request as ready for review May 30, 2025 11:15
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! Thanks for the cleanup. Related to the discussion here, I think we should not keep parameter from parent classes in docstring. Let's agree first on whether we want to keep them, and then, potentially, you can update this PR

@o-nikolas
Copy link
Contributor

Thanks for opening this! I'm still in favour of keeping the doc strings (I replied on the other thread I started that Vincent linked).

Copy link
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

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

Can you add tests for these operators like you did for the RDS case? I know not all of them have it, but it's a great mechanism to ensure we don't regress, so if you're modifying these ops anyway, adding the same tests would be great.

@dominikhei
Copy link
Contributor Author

Can you add tests for these operators like you did for the RDS case? I know not all of them have it, but it's a great mechanism to ensure we don't regress, so if you're modifying these ops anyway, adding the same tests would be great.

I added them for all the operators I have adjusted before + I have renamed the test: no_conn to default_conn to avoid any misunderstandings in the future. If u want me to add them to other operators etc. lmk!

@vincbeck vincbeck merged commit 2c44fdd into apache:main Jun 9, 2025
70 checks passed
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.

3 participants