github-actions[bot]
commented
3 weeks ago Open renanrodrigo opened 3 weeks ago
github-actions[bot]
commented
3 weeks ago 
renanrodrigo
commented
3 weeks ago Our autopkgtests are superficial, but overall they are all passing for:
For Xenial, we have passing autopkgtests for amd64 armhf ppc64el and s390x, but are failing to even start tests on arm64 and i386 - seems there is some problem in the infra itself. Let me know if you think that is a problem we need to solve our side.
renanrodrigo
commented
3 weeks ago I didn't care about Mantic as we are not releasing there. The estimated date for this release is end of July, when Mantic will be EOL
renanrodrigo
commented
2 weeks ago @lucaskanashiro FYI, this branch received (and may still receive) a couple test fixes and adjustments - no new functionality will be introduced though, we are just making sure all is good.
basak
commented
2 days ago I reviewed up to upstream commit cc151998, starting from upstream commit be597708 (tagged 32.3 upstream). Oracular is on 32.3.1. pkg/import/32.3 from git-ubuntu is identical to upstream tag 32.3, and git-ubuntu has further rich history with 2b115841 and e93055b0 being the material changes for 32.3.1. These I see in be597708..cc151998 as 95d11867 and 58d49cad respectively. git range-diff reports these commits as identical, so therefore apart from these two commits upstream, everything in be597708..cc151998 are the changes that would affect Ubuntu.
This was a bit of a pain to identify, FWIW. I wonder if we could make the process better next time? I don't mind where it is but what I want to review is precisely the set of (rich) git commits that will change from an Ubuntu user's perspective.
[Not for this release] 31dbcc73 fips: dynamically unhold packages to upgrade during auto-upgrade
I'm curious to understand why apt holds are being used for FIPS rather than apt pinning. This is surprising to me, but probably not a topic for this PR.
[Needs Information] 5bb896cb apt: use version string for comparison instead of version object
$ dpkg --compare-versions 1.2-ubuntu3.4 eq 1.2-ubuntu3.04 && echo equal equal
So version string comparisons cannot be used to determine equality in the general case. If such a version string gets normalised in some places but not others, then we would end up with a mismatch.
What's the reason for this change? Am I missing something? This is an example of why rationale should be in commit messages ☺
[Needs Information] fe6b23d4 Updated attach to check for an onlySeries field in the contract response
[Edit: I realised during the review that the issue here was fixed in a subsequent commit, but forgot to remove the comment, so I'm doing that now. Review time required is a good reason to follow a rebase workflow rather than a fixup workflow though for PR review iterations!]
[Comment only] 255d5a23 Add function to create timed job for checking release every 24 hours and check release on release-upgrade
It seems odd to me to do this on a timed job rather than on reboot. However, the latter would mean yet another service, so perhaps that's the reason? Since the timed job is only active when attached though, I don't think this is much of a concern from an SRU perspective.
lucaskanashiro
commented
2 days ago Well, based on Robie's comment, I believe the Pro client review process for PRs should also include checking for descriptive git commit messages. Good commit messages (describing how/what/why the changes are needed) would smooth the release process for everyone (reviewers will not seek for clarifications, and developers will not need to spend time explaining reviewers what they have in their minds). In this PR specifically, Robie could have approved (or requested changes) straightway if the information was there, reducing the back and forth that usually happens. Just my 2 cents.
renanrodrigo
commented
2 days ago @basak
This was a bit of a pain to identify, FWIW. I wonder if we could make the process better next time? I don't mind where it is but what I want to review is precisely the set of (rich) git commits that will change from an Ubuntu user's perspective.
We do have a better process, but this didn't follow the process as it's quite new. This was a separate upload done by @panlinux to hotfix an apparmor issue, and the new version will supersede the current one (we don't even mention it in the changelog). We agreed on that with Andreas so we could follow on with the release, and he'd have the freedom to upload the bugfix in the meantime. We could have anticipated and made it explicit in the PR though - a communication issue, we're sorry.
https://github.com/canonical/ubuntu-pro-client/commit/31dbcc73181d1e52e1de7a3e44346e0157279b75 fips: dynamically unhold packages to upgrade during auto-upgrade I'm curious to understand why apt holds are being used for FIPS rather than apt pinning. This is surprising to me, but probably not a topic for this PR.
It's easy - you can see we use the PIN for FIPS, and we never hold packages - we only unhold them (and then they are subject to the pinning we do). The reason FIPS packages may be pre-held is that CPC is creating FIPS cloud images without making use of the Pro Client in the process, and this is their solution. Further discussion welcome on a separate channel if you are interested in more.
I realised during the review that the issue here was fixed in a subsequent commit, but forgot to remove the comment, so I'm doing that now. Review time required is a good reason to follow a rebase workflow rather than a fixup workflow though for PR review iterations!]
We do have a rebase workflow, or rather a squash workflow, when issues and fixes are present in the same PR. This one was either a whoops we forgot to do it, OR it slipped through and we found it in a subsequent PR. Again, sorry for the inconvenience.
orndorffgrant
commented
1 day ago [Needs Information] 5bb896c apt: use version string for comparison instead of version object
$ dpkg --compare-versions 1.2-ubuntu3.4 eq 1.2-ubuntu3.04 && echo equal equal
So version string comparisons cannot be used to determine equality in the general case. If such a version string gets normalised in some places but not others, then we would end up with a mismatch.
What's the reason for this change? Am I missing something? This is an example of why rationale should be in commit messages ☺
It was a review suggestion as a sort of easy micro-optimization. We didn't realize equality couldn't be checked via string comparison (but of course now seeing the example, it makes sense that it can't). I'll revert it. Thanks for catching it!
[Comment only] 255d5a2 Add function to create timed job for checking release every 24 hours and check release on release-upgrade
It seems odd to me to do this on a timed job rather than on reboot. However, the latter would mean yet another service, so perhaps that's the reason? Since the timed job is only active when attached though, I don't think this is much of a concern from an SRU perspective.
A timer was the initial suggestion in the spec and was accepted because of the urgency of the feature and the ease of implementation. You're right though that it doesn't need to run except for on reboot. I've opened an issue to optimize this in a future version: #3189
basak
commented
23 hours ago Reviewed from my previous review point (cc151998) up to current main (1d72b42f) and all looks good. Thanks!
renanrodrigo
commented
22 hours ago Thanks much @basak! @lucaskanashiro shall we start the upload party?
renanrodrigo
commented
21 hours ago lucaskanashiro
commented
13 hours ago The content of main was just uploaded to Oracular. If everything goes well as expected, I'll upload the SRUs tomorrow.
This changeset represents the diff to be uploaded as version
33of theubuntu-pro-clientpackage.