-
Notifications
You must be signed in to change notification settings - Fork 75
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
Purge option for RepoEntitlements #2763
Conversation
Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference) GitHub Issues: No GitHub issues are fixed by this PR. (No commits have Fixes: #### references) Launchpad Bugs: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references) Documentation: The changes in this PR do require documentation changes, but those were not addressed yet. 👍 this comment to confirm that this is correct. |
9eab136
to
d7d4c4b
Compare
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 still need to review this more carefully, but these is the initial look
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.
Also, how should we treat reboot here ? We will always say to the user that we need a reboot after running the purge operation or should we check after each step to see if a package needs a reboot to list to the user which ones are causing the reboot operation
50abbd5
to
ceeb745
Compare
uaclient/entitlements/repo.py
Outdated
if not self.origin and self.purge: | ||
return ( | ||
False, | ||
CanDisableFailure( | ||
CanDisableFailureReason.NO_PURGE_WITHOUT_ORIGIN, | ||
messages.E_REPO_PURGE_FAIL_NO_ORIGIN.format( | ||
entitlement_name=self.title, title=self.title | ||
), | ||
), | ||
) |
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 is an error message for a bug if we, the developers, introduce a repo-entitlement without an origin. I wish we could structure the code in such a way that this is so unlikely as to not deserve a user-facing error message. But this is probably the right thing to do, I'm just complaining
ceeb745
to
90b6090
Compare
43be049
to
bc9ee25
Compare
Also we can add a warning telling this feature is experimental. We have many warnings, but this one should appear everywhere regardless. |
9376e7a
to
a44fbda
Compare
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 have tested this feature and it is working as expected. Just have a tiny nit, but overall LGTM
The "Origin" field in the apt metadata is what we use to identify whether a specific version for a package is part of what a service provides. Signed-off-by: Renan Rodrigo <[email protected]>
Signed-off-by: Renan Rodrigo <[email protected]>
This may exclude specific origins, and will be used in filtering which packages to remove and which packages to downgrade when purging RepoEntitlements Signed-off-by: Renan Rodrigo <[email protected]>
Signed-off-by: Renan Rodrigo <[email protected]>
this mimics what apt does, and enables us to clearly identify package lists in bigger chunks of text Signed-off-by: Renan Rodrigo <[email protected]>
perform_disable will check for the 'purge' flag, warn the user about the changed/removed packages, and perform the operations Signed-off-by: Renan Rodrigo <[email protected]>
Currently all RepoEntitlements have origins set, and thus can be purged based on that origin. If it happens that origin is not set, we cannot purge, and we block on can_disable. Signed-off-by: Renan Rodrigo <[email protected]>
Signed-off-by: Renan Rodrigo <[email protected]>
If a kernel package is part of the removal and we cannot detect any other Ubuntu kernel, block the operation. Otherwise, prompt the user. Signed-off-by: Renan Rodrigo <[email protected]>
Those messages indicate that packages will not be uninstalled Signed-off-by: Renan Rodrigo <[email protected]>
Signed-off-by: Renan Rodrigo <[email protected]>
Signed-off-by: Renan Rodrigo <[email protected]>
Signed-off-by: Renan Rodrigo <[email protected]>
a44fbda
to
48ab85c
Compare
Rebased. |
This is intended to land after #2744 - it contains the commits from there and will be rebased on whatever lands on that side.
still missing: integration tests
Why is this needed?
This PR solves all of our problems because it implements the purge option when disabling services involving apt repositories. This removes/downgrades all the packages from that specific source.
Test Steps
Integration tests still to come - but generally run disable for all services with
--purge
and see what happens :DChecklist
Does this PR require extra reviews?