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.

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

refactor: simplify prerm to not import python #2783

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Conversation

orndorffgrant
Copy link
Collaborator

@orndorffgrant orndorffgrant commented Oct 10, 2023

LP: #2021988

Why is this needed?

Importing our python module in prerm exposes the packaing script to all the complexity of our python and the risk that a seemingly unrelated change in the python could affect the prerm script.

This happened when we started relying on python3-apt and resulted in LP: #2021988

This PR removes the python import entirely and instead hardcodes the files that need to be removed.

Test Steps

./tools/test-in-lxd.sh xenial
pro attach
apt remove ubuntu-advantage-tools
ls /etc/apt/sources.list.d
# pro sources files should be gone

Checklist

  • I have updated or added any unit tests accordingly
  • I have updated or added any integration tests accordingly
  • Changes here need to be documented, and this was done in:

Does this PR require extra reviews?

  • Yes
  • No

@github-actions
Copy link

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:

Documentation: The changes in this PR do not require documentation changes.

👍 this comment to confirm that this is correct.

@orndorffgrant orndorffgrant marked this pull request as ready for review October 13, 2023 19:16
Copy link
Collaborator

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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.

LGTM, I just think there is one service missing

'

# This list should be kept up to date with the list of available apt-repo-based services
for service in anbox cc-eal cis esm-apps esm-infra fips fips-preview fips-updates realtime-kernel ros ros-updates; do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also need usg here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It won't hurt, but is the apt source file named with the presented_as name?

Copy link
Member

Choose a reason for hiding this comment

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

nope, source-file and key will be ubuntu-cis.list unfortunately 🤡 :upset_cry:

Copy link
Member

@renanrodrigo renanrodrigo left a comment

Choose a reason for hiding this comment

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

/methinks we can merge this

@@ -661,43 +660,6 @@ def remove_apt_list_files(repo_url, series):
system.ensure_file_absent(path)


def clean_apt_files(*, _entitlements=None):
Copy link
Member

Choose a reason for hiding this comment

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

removing code and keeping functionality? good good

@lucasmoura
Copy link
Contributor

CI errors are unrelated

@lucasmoura lucasmoura merged commit 3473ef2 into main Oct 17, 2023
26 of 27 checks passed
@lucasmoura lucasmoura deleted the simple-prerm branch October 17, 2023 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants