Skip to content

Conversation

@vincbeck
Copy link
Contributor

PubliclyAccessible needs to be always specified because the default value (if not provided) is True on Redshift service side


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Jul 18, 2024
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.

I might have added a comment to say why it is different from the rest, but I'll approve it. Thanks for fixing this.

@vincbeck
Copy link
Contributor Author

I might have added a comment to say why it is different from the rest, but I'll approve it. Thanks for fixing this.

If PubliclyAccessible is not provided to the service, it considers it as True. Thus, when passing False to publicly_accessible, with the current code, it does not set it, which is then considered as True for Redshift. Removing the if solves that issue

if self.publicly_accessible:
            params["PubliclyAccessible"] = self.publicly_accessible

@ferruzzi
Copy link
Contributor

ferruzzi commented Jul 18, 2024

I get it, but just looking at the code it is not clear why that one value isn't wrapped in an if like all the others are. No biggie, I over-comment everything :P

@vincbeck
Copy link
Contributor Author

I get it, but just looking at the code it is not clear why that one value isn't wrapped in an if like all the others are. No biggie, I over-comment everything :P

Oh! Get it :) That's a good point, adding a comment

@ferruzzi
Copy link
Contributor

I can just see someone in the future looking at that operator and deciding to clean it up and not even noticing thta one parameter is handled differently; even worse, now that I think of it, the tests would all still pass if they did unintentionally revert that in an effort to standardize or clean up that cascading mess of if statements....

@vincbeck vincbeck merged commit 06b19eb into apache:main Jul 18, 2024
@vincbeck vincbeck deleted the vincbeck/example_redshift branch July 18, 2024 19:56
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
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