-
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
Some bits missing in the deb822 transition #2916
Conversation
Signed-off-by: Renan Rodrigo <[email protected]>
Signed-off-by: Renan Rodrigo <[email protected]>
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 not require documentation changes. 👍 this comment to confirm that this is correct. |
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.
Just an idea to avoid code duplication, let me know what you think
uaclient/entitlements/base.py
Outdated
old_sources_path = "/etc/apt/sources.list" | ||
new_sources_path = "/etc/apt/sources.list.d/ubuntu.sources" | ||
sources_file = ( | ||
new_sources_path | ||
if os.path.exists(new_sources_path) | ||
else old_sources_path | ||
) | ||
apt.update_sources_list(sources_file) |
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.
Just to avoid code duplication, what about if we make this logic the default sources_file
we if we don't pass anything to update_sources_list
?
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.
right
any chance of a situation where we just want to update all sources, pro or not, without specifying a file?
Doesn't exist right now but I wonder if we should account for that
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.
Good point, what if we create a shell function that just calls update_sources_list
with the /etc/
path ? Then we would still be able to extend the update_sources_list
function to the use case you mentioned, while also avoiding the code duplication.
What do you think ?
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 thought that would be overcomplicating it - deduplicated with an apt util function to get the file. Let me know if that suffices
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 like this solution. @lucasmoura please merge if you're okay with it
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.
That works for me. Thanks @renanrodrigo
Signed-off-by: Renan Rodrigo <[email protected]>
cd4538c
to
7f31d64
Compare
newlines indicate new stanzas, there is no need to confuse python3-apt Signed-off-by: Renan Rodrigo <[email protected]>
@orndorffgrant @lucasmoura added a new commit with a small tweak, double check before clicking |
Why is this needed?
This PR solves all of our problems because it brings small tweaks needed for the deb822 transition, both in tests and functionality bits.
Test Steps
This one is interesting... the integration test changes are fine to check, but the other ones can really be tested when noble changes the format in images :D
Checklist
Does this PR require extra reviews?