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

Oracular update for apt-hook #3365

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Oracular update for apt-hook #3365

wants to merge 1 commit into from

Conversation

dheyay
Copy link
Contributor

@dheyay dheyay commented Nov 5, 2024

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


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

Copy link

github-actions bot commented Nov 5, 2024

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

@dheyay dheyay marked this pull request as draft November 6, 2024 17:52
@dheyay dheyay marked this pull request as ready for review November 6, 2024 23:37
@orndorffgrant orndorffgrant changed the base branch from next-v36 to main December 19, 2024 15:40
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.

Sorry for the very long delay.

@dheyay dheyay force-pushed the orc-hook-update branch 2 times, most recently from fc41ebc to a2d21db Compare January 9, 2025 20:23
@dheyay
Copy link
Contributor Author

dheyay commented Jan 9, 2025

Updated @orndorffgrant

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 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

@dheyay
Copy link
Contributor Author

dheyay commented Jan 14, 2025

@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

@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.

@dheyay
Copy link
Contributor Author

dheyay commented Jan 15, 2025

Updated with the test @lucasmoura @orndorffgrant

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.

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.

@dheyay dheyay force-pushed the orc-hook-update branch 4 times, most recently from ce5e414 to 70fbf54 Compare January 21, 2025 22:03
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.

I'm still running all the affected integration tests, but noticed two things.

@dheyay dheyay force-pushed the orc-hook-update branch 2 times, most recently from e2ee22b to f56998f Compare January 22, 2025 17:04
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!

@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.

Copy link
Contributor

@julian-klode julian-klode left a 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").

@julian-klode
Copy link
Contributor

julian-klode commented Jan 27, 2025

You can now also query for plucky onward the binary::apt::APT::Output-Version option in the options array that's part of the hello handshake.

It is an integer (possibly stored in a string), 30 is the new format (it's essentially major*10+minor.

And quiet is an integer of 1 for quieter, or 2 for "no output".

@renanrodrigo
Copy link
Member

@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.

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.

5 participants