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.

Sign up for GitHub

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

Jump to bottom

Use packaging for version parsing. looseversion when needed only. #63383

Merged
merged 6 commits into from
Jan 9, 2023
Merged

Use packaging for version parsing. looseversion when needed only. #63383

merged 6 commits into from
Jan 9, 2023

Conversation

s0undt3ch
Copy link
Collaborator

What does this PR do?

See title

@s0undt3ch s0undt3ch force-pushed the hotfix/loose-version branch 10 times, most recently from 142fdf9 to 3e0899b Compare December 30, 2022 07:29
@s0undt3ch s0undt3ch marked this pull request as ready for review December 30, 2022 13:10
@s0undt3ch s0undt3ch requested a review from a team as a code owner December 30, 2022 13:10
@s0undt3ch s0undt3ch requested review from twangboy and removed request for a team December 30, 2022 13:10
twangboy
twangboy previously approved these changes Jan 3, 2023
View reviewed changes
Ch3LL
Ch3LL suggested changes Jan 3, 2023
View reviewed changes
requirements/base.txt Show resolved Hide resolved
salt/utils/versions.py Show resolved Hide resolved
salt/utils/versions.py Show resolved Hide resolved
salt/modules/libcloud_dns.py Show resolved Hide resolved
@Ch3LL Ch3LL added the Sulfur v3006.0 release code name and version label Jan 3, 2023
@Ch3LL Ch3LL requested a review from dmurphy18 January 4, 2023 19:13
@dmurphy18
Copy link
Contributor

re-run full all

dmurphy18
dmurphy18 reviewed Jan 5, 2023
View reviewed changes
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

Wondering about that comparison to needed_version, might be a logic error in the original Salt.

salt/modules/file.py Show resolved Hide resolved
salt/utils/versions.py Show resolved Hide resolved
@s0undt3ch s0undt3ch force-pushed the hotfix/loose-version branch 3 times, most recently from 8ed2f2d to f9fcc26 Compare January 6, 2023 15:43
@s0undt3ch s0undt3ch requested a review from Ch3LL January 6, 2023 15:50
@s0undt3ch
Copy link
Collaborator Author

re-run full all

Ch3LL
Ch3LL previously approved these changes Jan 6, 2023
View reviewed changes
@s0undt3ch s0undt3ch requested a review from Ch3LL January 6, 2023 19:52
@s0undt3ch s0undt3ch force-pushed the hotfix/loose-version branch 4 times, most recently from 44eecdf to 506af57 Compare January 7, 2023 12:22
@s0undt3ch s0undt3ch force-pushed the hotfix/loose-version branch 2 times, most recently from c243975 to 7508c70 Compare January 8, 2023 13:36
@s0undt3ch
Use packaging for version parsing. looseversion when needed only.
d361324 
Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>
@s0undt3ch
Properly deprecate salt.utils.versions.StrictVersion
467f310 
Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>
@s0undt3ch
Use the latest Python 3.8 docker image
fd4fb14 
Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>
@s0undt3ch
Stop relying on salt/_version.py to write Salt's version.
94e7c80 
Instead use `salt/_version.txt` which only contains the version string.

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>
@s0undt3ch
Remove code no longer required
7f5cd8f 
Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>
@s0undt3ch
Address new lint issues
b38fade 
Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>
Ch3LL
Ch3LL reviewed Jan 9, 2023
View reviewed changes
Copy link
Contributor

@Ch3LL Ch3LL left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just one question:

Are we still generating _version.py. If not, my concern is that other people are relying on this file.

@s0undt3ch
Copy link
Collaborator Author

Just one question:

Are we still generating _version.py. If not, my concern is that other people are relying on this file.

No, the only generated file now is _version.txt.

As for someone using _version.py, the file is _ prefixed already suggesting private and we never said anyone should get Salt's version for that file, only from version.py.

So, adjust their workflow if anyone is relying on _version.py.

Happy to be to convinced otherwise.

dmurphy18
dmurphy18 reviewed Jan 9, 2023
View reviewed changes
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

We need to specify what the format of _version.txt is somewhere, to ensure in the future that the file is being written correctly.
Do not see anywhere specification for the format of _version.txt, relying on code to document, doesn't work since code can have errors and be modified incorrectly.

salt/utils/pkg/win.py Show resolved Hide resolved
salt/utils/versions.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
@s0undt3ch
Copy link
Collaborator Author

We need to specify what the format of _version.txt is somewhere, to ensure in the future that the file is being written correctly. Do not see anywhere specification for the format of _version.txt, relying on code to document, doesn't work since code can have errors and be modified incorrectly.

There is no format, its the str() cast of and instance of SaltStackVersion. Salt always knows how to parse it.

@Ch3LL Ch3LL merged commit e8555a0 into saltstack:master Jan 9, 2023
@s0undt3ch s0undt3ch deleted the hotfix/loose-version branch January 9, 2023 19:45
@dafyddj dafyddj mentioned this pull request Mar 23, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmurphy18 dmurphy18 dmurphy18 approved these changes

@Ch3LL Ch3LL Ch3LL approved these changes

@whytewolf whytewolf whytewolf approved these changes

@twangboy twangboy Awaiting requested review from twangboy

Assignees
No one assigned
Labels
Sulfur v3006.0 release code name and version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@s0undt3ch @dmurphy18 @whytewolf @Ch3LL @twangboy