Skip to content

Conversation

@o-nikolas
Copy link
Contributor

#34747 upgraded the Amazon Provider Package to support Watchtower V3. The requirements were updated to only install v3 versions of watchtower, but many users may still need to use V2 (see example thread), which should still be perfectly working with the provider package code base (unless I'm mistaken, please correct me!).

This PR allows a wider range of watchtower versions to be installed along with the amazon provider package.


^ 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.

@eladkal
Copy link
Contributor

eladkal commented Nov 17, 2023

Is the added config of cloudwatch_task_handler_json_serializer relevant for watchtower<3 ? If not we should also edit the description of the setting with note about it to avoid confusion

@o-nikolas
Copy link
Contributor Author

Is the added config of cloudwatch_task_handler_json_serializer relevant for watchtower<3 ? If not we should also edit the description of the setting with note about it to avoid confusion

Good question @eladkal!

I did check before submitting the PR that the 2.0.1 version of the Watchtower code to ensure that parameter is accepted and it looks like it is: https://github.com/kislyuk/watchtower/blob/v2.0.1/watchtower/__init__.py#L185

The default serializer we provide was a fallback for the one WT uses anyways, and providing a new one should still work just as intended with 2.0.1. So I think it's all good. But maybe @cBiscuitSurprise can confirm just in case.

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.

Code wise, it looks good to me. Though, probability of breaking things here is pretty unclear to me :)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants