Skip to content

Conversation

@eladkal
Copy link
Contributor

@eladkal eladkal commented Oct 22, 2022

Dropping support for resource parameter
Deprecation added in #24673


^ 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 eladkal requested review from dstandish and potiuk October 22, 2022 09:41
@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers labels Oct 22, 2022
Copy link
Contributor

@dstandish dstandish left a comment

Choose a reason for hiding this comment

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

left comment but approving anyway

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.

Cool. Test needs fixing though.

@eladkal eladkal force-pushed the k8s branch 3 times, most recently from 6429678 to 1251b58 Compare October 27, 2022 07:36
@dstandish
Copy link
Contributor

Ran the test locally on this branch (using virtualenv + docker desktop)

Saw this is the error diff:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

should we also remove this module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find any more usage in the code to convert_resources so I removed it.
I think we should remove the whole backward compact module but it needs more close look on the other functions

@dstandish
Copy link
Contributor

dstandish commented Oct 28, 2022

ok so to fix.... first of all, actual vs expected needs to be flipped in those asserts

then this needs to be removed from the "expeced"

                        "resources": {},

(really just remove that line should do it, can leave the assert flipping for another time since it's tough for you to test locally)

@eladkal eladkal requested a review from dstandish October 28, 2022 19:59
@eladkal
Copy link
Contributor Author

eladkal commented Oct 28, 2022

CI is green :)
@dstandish can you review?

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

Labels

area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants