Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RM-36 bump tenacity version #207

Conversation

ryantimjohn
Copy link
Contributor

No description provided.

@ryantimjohn ryantimjohn marked this pull request as ready for review February 6, 2023 21:49
@ryantimjohn ryantimjohn requested a review from Brunope February 6, 2023 21:49
@ryantimjohn
Copy link
Contributor Author

@Brunope, seems like this just worked.

@Brunope
Copy link
Collaborator

Brunope commented Feb 7, 2023

Have you tested this specifically with airflow >= 2.2.2, since that was the reason for this upgrade? The setup.py still says ">=2.0". If you have, looks good!

@ryantimjohn
Copy link
Contributor Author

Screenshot 2023-02-08 at 10 57 07 AM
Yep! Will chat in our call today just to make sure I understand @archetypalsxe 's original ticket!

@ryantimjohn
Copy link
Contributor Author

Related to #197

@ryantimjohn
Copy link
Contributor Author

Okay... this is dumb. So, the coverage is different between different versions of python with this particular version of Airflow. 😠

Otherwise this passes.

I'm going to revert to >=2.0; if this ever comes up again, we'd have to update this class to be able to handle ranges:

records-mover/setup.py

Lines 39 to 73 in 9167f38

class CoverageRatchetCommand(Command):
description = 'Run coverage ratchet'
user_options = [] # type: ignore
coverage_file: str
coverage_source_file: str
coverage_url: str
type_of_coverage: str
def finalize_options(self) -> None:
pass
def run(self) -> None:
"""Run command."""
import xml.etree.ElementTree as ET
tree = ET.parse(self.coverage_source_file)
new_coverage = Decimal(tree.getroot().attrib["line-rate"]) * 100
if not os.path.exists(self.coverage_file):
with open(self.coverage_file, 'w') as f:
f.write('0')
with open(self.coverage_file, 'r') as f:
high_water_mark = Decimal(f.read())
if new_coverage < high_water_mark:
raise Exception(
f"{self.type_of_coverage} coverage used to be {high_water_mark}; "
f"down to {new_coverage}%. Fix by viewing '{self.coverage_url}'")
elif new_coverage > high_water_mark:
with open(self.coverage_file, 'w') as f:
f.write(str(new_coverage))
print(f"Just ratcheted coverage up to {new_coverage}%")
else:
print(f"Code coverage steady at {new_coverage}%")

Or find a different way to handle ratcheting and all that which didn't rely on development in-house.

@Brunope
Copy link
Collaborator

Brunope commented Feb 10, 2023

I'm pretty annoyed by the ratchet coverage tests too, see this PR #209 where changing a version from 39.0.0 to 39.0.1 breaks the coverage test.

RM-55 update to warn when below high water mark

RM-55 apply autopep8

RM-55 shorten line

Revert "RM-55 test lowering mypy high water mark"

This reverts commit 099e56c.
…o RM-36-Upgrade-tenacity-version-8.0.1-to-support-Airflow-2.2.2
setup.py Show resolved Hide resolved
@ryantimjohn ryantimjohn merged commit cff0fb8 into master Feb 17, 2023
@ryantimjohn ryantimjohn deleted the RM-36-Upgrade-tenacity-version-8.0.1-to-support-Airflow-2.2.2 branch February 17, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants