-
Notifications
You must be signed in to change notification settings - Fork 78
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
Oracular update for apt-hook #3365
base: main
Are you sure you want to change the base?
Conversation
PR ChecklistHow to use this checklistHow to use this checklistPR AuthorFor each section, check a box when it is true. PR ReviewerCheck that the PR checklist action did not fail. Bug References
Confirm
How to properly reference fixed bugs
Test UpdatesUnit Tests
Integration Tests
Documentation
Does this PR require review from someone outside the core ubuntu-pro-client team?
|
9589482
to
898f2cd
Compare
1374aac
to
3676b47
Compare
3676b47
to
3fee5da
Compare
3fee5da
to
26b36be
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.
Sorry for the very long delay.
fc41ebc
to
a2d21db
Compare
Updated @orndorffgrant |
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.
@dheyay I know that you are adding a test for APT news, but I wonder if we should add an orocular integration test for the json-hook
changes
a2d21db
to
6151922
Compare
@lucasmoura I would say we can probably do without a dedicated test since the updated test acts like a dedicated oracular test for the json-hook. It checks the formatting for the apt-news message as well as the summary for the number of packages. Let me know if that works, I would not mind adding a short test to check for formatting of updates/packages as well. |
Updated with the test @lucasmoura @orndorffgrant |
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.
Please run the hook unit tests: make -C apt-hook test
(Github CI is failing on package builds because the build runs those tests).
Also:
- I've properly referenced all bugs that this PR fixes
is not accurate since the comment displays "None" under "Bug references". Please reference the LP bug in a commit in the format "LP: #12345678" (No "Fixes").
Also please clean up the commits in the PR. These can probably all be squashed into one.
ce5e414
to
70fbf54
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'm still running all the affected integration tests, but noticed two things.
318db6d
to
4d12f38
Compare
e2ee22b
to
f56998f
Compare
LP: #2086753
f56998f
to
6a48332
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.
Thanks!
@julian-klode Can you also do a quick review of this?
This PR shows reformatted pro messages and APT News to fit with the new APT UX on oracular onward. It determines what style to show by the release in /etc/os-release.
I know that ideally the style would be determined by APT telling the hook what style it is using via jsonrpc params, but we'd like to get something basic done in our v35 release in February.
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 looks good. It might be worth thinking about realigning
"%d standard security updates"
to
"Standard security updates: %d"
to match the new summary format ordering ("X upgraded" became "Upgrading: X").
You can now also query for plucky onward the It is an integer (possibly stored in a string), 30 is the new format (it's essentially major*10+minor. And |
@dheyay Thanks much for your work. As @julian-klode says in plucky we get to read the option, it's better if we fix it for plucky onward. I will take this PR when v35 is released and this will be part of the next release. |
Why is this needed?
This PR solves all of our problems because it correctly formats apt news and apt messages for oracular.
Fixes: LP #2086753
Test Steps