-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
Maybe we can add some unit tests for this if they don't already exist. |
There was a problem hiding this 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.
There was a problem hiding this 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): |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
@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. |
Reconfigure how the scheduler considers version in the context of comparing workflows.