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

Do not use pre-release versions as is_highest unless no stable version exists. #1392

Merged
merged 10 commits into from Mar 20, 2023
Merged

Do not use pre-release versions as is_highest unless no stable version exists. #1392

merged 10 commits into from Mar 20, 2023

Conversation

jctanner
Copy link
Contributor

@jctanner jctanner commented Mar 9, 2023

fixes: #1391

@jctanner
Do not use pre-release versions as is_highest unless no stable versio…
1e866ee 
…ns exist.

fixes: pulp#1391

Signed-off-by: James Tanner <tanner.jc@gmail.com>
gerrod3
gerrod3 requested changes Mar 10, 2023
View reviewed changes
pulp_ansible/app/tasks/collections.py Outdated Show resolved Hide resolved
@jctanner
Add a param for saving on the incoming version or not.
433e7c1 
fixes: pulp#1391

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@jctanner
Add test and logic for edge case when computing higest and mulitple p…
d9dcf06 
…re-releases exist.

fixes: pulp#1391

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@jctanner jctanner requested a review from gerrod3 March 15, 2023 16:28
mdellweg
mdellweg reviewed Mar 15, 2023
View reviewed changes
pulp_ansible/app/tasks/collections.py Outdated Show resolved Hide resolved
pulp_ansible/app/tasks/collections.py Outdated Show resolved Hide resolved
pulp_ansible/app/tasks/collections.py Outdated Show resolved Hide resolved
pulp_ansible/app/tasks/collections.py Outdated Show resolved Hide resolved
pulp_ansible/app/tasks/collections.py Outdated Show resolved Hide resolved
@jctanner
Review comments.
f895ea7 
fixes: pulp#1391

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@jctanner
More tests
f08893a 
fixes: pulp#1391

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@jctanner
Revise.
8987f39 
fixes: pulp#1391

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@jctanner
Revise.
5c6a6ec 
fixes: pulp#1391

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@jctanner jctanner requested a review from mdellweg March 17, 2023 23:38
mdellweg
mdellweg reviewed Mar 20, 2023
View reviewed changes
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Just one last question before i think we are ready to merge.

pulp_ansible/app/tasks/collections.py
@@ -421,7 +421,7 @@ def _get_backend_storage_url(artifact_file):
return url


def _update_highest_version(collection_version):
def _update_highest_version(collection_version, save=False):
Copy link
Member

Choose a reason for hiding this comment

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

I fail to see the use for save=False. Don't we always want to update when we go to the lengths of running this function at all?

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 addressed this in a previous review comment. @gerrod3 wants to retain the behavior where the function caller does the saving when possible to reduce duplicate saving.

Copy link
Contributor

Choose a reason for hiding this comment

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

We call this method in two places:

In both cases we save the incoming collection_version after calling the method.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, so we are actually preventing a superfluous save call.

@jctanner
Revise.
c4b6787 
fixes: pulp#1391

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@jctanner jctanner requested a review from gerrod3 March 20, 2023 15:07
gerrod3
gerrod3 requested changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@gerrod3 gerrod3 left a comment

Choose a reason for hiding this comment

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

Last changes required (I think). Makes it a bit more performant. https://docs.djangoproject.com/en/3.2/ref/models/instances/#specifying-which-fields-to-save

pulp_ansible/app/tasks/collections.py Outdated Show resolved Hide resolved
pulp_ansible/app/tasks/collections.py Outdated Show resolved Hide resolved
pulp_ansible/app/tasks/collections.py Outdated Show resolved Hide resolved
mdellweg
mdellweg approved these changes Mar 20, 2023
View reviewed changes
pulp_ansible/app/tasks/collections.py
@@ -421,7 +421,7 @@ def _get_backend_storage_url(artifact_file):
return url


def _update_highest_version(collection_version):
def _update_highest_version(collection_version, save=False):
Copy link
Member

Choose a reason for hiding this comment

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

Thank you, so we are actually preventing a superfluous save call.

@jctanner
Revise.
865993f 
fixes: pulp#1391

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@jctanner
Lint fix.
e5758a1 
fixes: pulp#1391

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@jctanner jctanner requested a review from gerrod3 March 20, 2023 18:17
@gerrod3 gerrod3 merged commit 8ad72ba into pulp:main Mar 20, 2023
13 checks passed
@briantist briantist mentioned this pull request Sep 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@newswangerd newswangerd newswangerd approved these changes

@mdellweg mdellweg mdellweg approved these changes

@gerrod3 gerrod3 gerrod3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

semantic_version says pre-release versions can be higher than release versions
4 participants
@jctanner @newswangerd @mdellweg @gerrod3