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

compare vversion in the schduler #63

Closed
wants to merge 3 commits into from
Closed

Conversation

Michal-Babins
Copy link

Reconfigure how the scheduler considers version in the context of comparing workflows.

@Michal-Babins Michal-Babins requested a review from scanon February 21, 2024 21:03
@scanon
Copy link
Collaborator

scanon commented Feb 22, 2024

Maybe we can add some unit tests for this if they don't already exist.

Copy link
Collaborator

@scanon scanon left a comment

Choose a reason for hiding this comment

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

I think this looks good, but we should add some tests and confirm that the behavior is the same before and after.

Copy link
Collaborator

@scanon scanon left a comment

Choose a reason for hiding this comment

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

Need to add one test and confirm if things are working as expected.

if v1.major == v2.major and v1.minor == v2.minor:
return v1 == v2

if getattr(v1, version_range) == getattr(v2, version_range):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would just fetch the major minor or patch of the version right? It seems like this isn't quite right.

Do you have a test that is something like 1.1, 2.1, "minor"? I'm thinking this would match when it shouldn't.

@@ -32,25 +32,29 @@ def get_mongo_db() -> MongoDatabase:
return _client[os.getenv("MONGO_DBNAME")]


def within_range(wf1: Workflow, wf2: Workflow, force=False) -> bool:
def within_range(wf1: Workflow, wf2: Workflow, version_range: str, force: bool = False) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a test and raise a ValueError if version_range isn't in the allowed options.

@aclum
Copy link
Contributor

aclum commented Jul 16, 2024

@scanon can we close this w/o merging since you've been fixing bugs in the version schedule comparisons?

@scanon
Copy link
Collaborator

scanon commented Dec 11, 2024

@scanon can we close this w/o merging since you've been fixing bugs in the version schedule comparisons?

I think so. I will close it now.

@scanon scanon closed this Dec 11, 2024
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.

3 participants