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

Apt pinned packages #3297

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Apt pinned packages #3297

merged 1 commit into from
Oct 2, 2024

Conversation

dheyay
Copy link
Contributor

@dheyay dheyay commented Sep 6, 2024 •
edited
Loading

Why is this needed?

Fix for #3184
Documentation update PR: #3328

Test Steps

  • (un)check this to re-run the checklist action

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 6, 2024 •
edited by dheyay
Loading

PR Checklist

How to use this checklist

How to use this checklist

PR Author

For each section, check a box when it is true.
Uncheck a box if it becomes un-true.
Then check the box at the bottom of the PR description to re-run the action that creates this checklist.
The action that creates and updates this comment will retain your edits.
The action will fail if the checklist is not completed.

PR Reviewer

Check that the PR checklist action did not fail.
Double check that the author filled out the checklist accurately.
If you disagree with a checklist item, start a conversation.
For example, the author may say they don't think integration tests are necessary, but you may disagree.

Bug References

Confirm

  • I've properly referenced all bugs that this PR fixes
How to properly reference fixed bugs
  • If this PR is related to a Jira item, include an SC-1234 reference in the PR title
  • If this PR is fixes a GitHub issue, include a Fixes: #1234 reference in the commit that fixes the issue
  • If this PR is fixes a Launchpad bug, include a LP: #12345678 reference in the commit that fixes the issue

Test Updates

Unit Tests

  • I have updated or added any unit tests accordingly
  • No unit test changes are necessary for this change

Integration Tests

  • I have updated or added any integration tests accordingly
  • No integration test changes are necessary for this change

Documentation

  • Changes here need to be documented and I have referenced the docs PR in the description
  • No documentation updates are necessary for this change

Does this PR require review from someone outside the core ubuntu-pro-client team?

  • Yes, and I have requested those reviews via GitHub
  • No

renanrodrigo
renanrodrigo reviewed Sep 10, 2024
View reviewed changes
uaclient/security_status.py Outdated Show resolved Hide resolved
uaclient/security_status.py Outdated Show resolved Hide resolved
@dheyay dheyay force-pushed the apt-pinned-packages branch 2 times, most recently from 7ba01f5 to 20a23de Compare September 10, 2024 23:06
@dheyay dheyay marked this pull request as ready for review September 10, 2024 23:15
@dheyay dheyay changed the title wip: Apt pinned packages Apt pinned packages Sep 11, 2024
@dheyay dheyay force-pushed the apt-pinned-packages branch 2 times, most recently from 2773ad8 to 032d973 Compare September 12, 2024 20:42
lucasmoura
lucasmoura reviewed Sep 20, 2024
View reviewed changes
Copy link
Contributor

@lucasmoura lucasmoura left a comment

Choose a reason for hiding this comment

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

@dheyay @renanrodrigo do you think we could come up with an integration test for this ?

uaclient/security_status.py Show resolved Hide resolved
orndorffgrant
orndorffgrant requested changes Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@orndorffgrant orndorffgrant left a comment

Choose a reason for hiding this comment

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

As part of our process to get ready for v35, I have rebased next-v35 onto main. As a result, next-v35 is now obsolete. Please rebase this on main and target the PR to main.

uaclient/security_status.py Outdated Show resolved Hide resolved
@dheyay dheyay force-pushed the apt-pinned-packages branch 2 times, most recently from 0c40fe1 to c909222 Compare September 25, 2024 16:15
@dheyay dheyay changed the base branch from next-v35 to main September 25, 2024 16:15
renanrodrigo
renanrodrigo reviewed Sep 26, 2024
View reviewed changes
uaclient/security_status.py Outdated
if is_candidate
else UpdateStatus.AVAILABLE_NOT_PREFERRED.value
)
if ua_info["attached"] and service_name in ua_info["enabled_services"]:
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason to apply the logic only to the archive, but not to possible underpreferred ESM versions?
I mean, even if those are not a planned thing, they may happen and the criteria is the same - anything available but not a candidate should be in the new state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this as per standup discussion 😄

@renanrodrigo
Copy link
Member

Writing the integration test would be hard (as you probably figured out by now) because the situation where we have unpreferred updates is (or should be) temporary (see the issue - the package which triggered it is fixed already). Either we fake it all (no reason to have an integration test then, just the unit test will do) or we reason about it and move on.

In the second case, I would still go for a manual test, even if i involves manipulating apt caches, just to make sure.

orndorffgrant
orndorffgrant reviewed Sep 30, 2024
View reviewed changes
Copy link
Collaborator

@orndorffgrant orndorffgrant left a comment

Choose a reason for hiding this comment

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

Thanks this looks good! Approval except for below:

From the checklist:

  • No documentation updates are necessary for this change

This introduces a new enum value, which must at least be described here: https://canonical-ubuntu-pro-client.readthedocs-hosted.com/en/latest/explanations/how_to_interpret_the_security_status_command/#u-pro-packages-updates-v1

uaclient/security_status.py Outdated
def _is_candidate_version(pkg: str, version: str) -> bool:
"""Returns True if the package version is a candidate version."""
candidate_version = get_pkg_candidate_version(pkg, check_esm_cache=False)
print(pkg, " ", version, " ", candidate_version)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
print(pkg, " ", version, " ", candidate_version)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and created #3328

@dheyay dheyay mentioned this pull request Sep 30, 2024
Merged
1 task
@orndorffgrant orndorffgrant merged commit 0ef5056 into main Oct 2, 2024
12 of 16 checks passed
@orndorffgrant orndorffgrant deleted the apt-pinned-packages branch October 2, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@renanrodrigo renanrodrigo renanrodrigo left review comments

@orndorffgrant orndorffgrant orndorffgrant approved these changes

@lucasmoura lucasmoura lucasmoura approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@dheyay @renanrodrigo @lucasmoura @orndorffgrant