Merge ~renanrodrigo/ubuntu/+source/ubuntu-advantage-tools:upload-30-mantic into ubuntu/+source/ubuntu-advantage-tools:ubuntu/devel

It is a bit confusing that only 10 commits are listed on this MP, but there seem to be a lot more referenced in d/changelog, and indeed `git log pkg/import/29.. | grep ^commit | wc -l` shows 185 commits?

Below is review on the 10 from this MP, the other 175 will take me awhile assuming they also need reviewed. Nothing major (these 10 are pretty straightforward) but a couple questions/fixups:

### Commit-by-Commit Review ###

15f4506... by Renan Rodrigo on 2023-10-03
    messages: add gettext to new purge messages

+1 Simple refactor to add gettext() calls

------------------------------------------------------------------------
573c3ec... by Renan Rodrigo on 2023-10-03
    disable: messages to warn that --purge is experimental

- Changelog entry should be "messages that warn that --purge"

- For the warning itself, I'd suggest a slightly different wording:

  +PURGE_EXPERIMENTAL = t.gettext(
  + "(The --purge flag is still experimental - use with caution)"
  +)

------------------------------------------------------------------------
629fc4d... by Renan Rodrigo on 2023-10-03
    po: run tools/update-pos.sh

+1 (Just translations)

------------------------------------------------------------------------
fa7505d... by Renan Rodrigo 5 hours ago
    fix: reposition message to avoid po conflicts

+1 (Trivial refactor, no actual code change)

------------------------------------------------------------------------
9e5cfc2... by Renan Rodrigo 16 hours ago
    chore: archive release-29 sru scripts
    also create a folder for release-30 and let the esm pinning test rest
    there

+1 (Just directory housekeeping)

------------------------------------------------------------------------
9bb2be3... by Renan Rodrigo 20 hours ago
    esm: pin repositories to a higher priority using static files
    This reverts commit 1dfcade40602ff820017bdf6b260b066e77f740d.

? I grepped the codebase for '500' for any other pinning instances.
   The following files seem to all be tests, and may not be app or infra
   related, so I'm guessing these are not affected, but can you confirm?

  ./features/anbox.feature
  ./features/attached_enable.feature
  ./features/realtime_kernel.feature
  ./uaclient/tests/test_apt.py
  ./uaclient/entitlements/tests/test_repo.py
  ./apt-hook/json-hook.test.cc

------------------------------------------------------------------------
a8e1aca... by Renan Rodrigo 20 hours ago
    esm: add comment to preference files
    This reverts commit 71aab656ea44aa1421fa59c8fec3d6935d9ec6c7.

+1 Comments in a preferences config file, entirely replaced in next
    commit...

------------------------------------------------------------------------
bef8bad... by Renan Rodrigo 20 hours ago
    Update esm pin file comment
    This reverts commit a8d66f68088c267cdf7a6f5a114547f4a3360fc5.

+1 Comments in a preferences config file, so safe. No errors in
    grammar, and the changes read ok.

I'm curious though why this isn't squashed with a8e1aca?

------------------------------------------------------------------------
a334bcf... by Lucas Albuquerque Medeiros de Moura 4 hours ago
    fips-preview: add gpg key for the service
    To enable ...

You are correct, 154 commits should be part of the MR... I wonder what did happen here - will try to fix it up

Nothing to fix it seems \_("/)_/

Thanks for doublechecking, I suppose Launchpad is the thing needing fixed in this case...

I've performed the required commit-by-commit review, in particular focusing on the types of changes identified by https://wiki.ubuntu.com/UbuntuAdvantageToolsUpdates - apt, systemd, config files, etc. Due to the number of commits in this release, the full report is pretty long, so rather than paste it into Launchpad I put it in Ubuntu's pastebin, which hopefully will be easier to read, copy, and/or download.

    https://paste.ubuntu.com/p/fQYm9QfGWm/

Hopefully this works ok for you. I have flagged all the actual notes with 'FIXME' tags for your search convenience, though as you'll see in some cases it's just questions or reminders on things to do or check later.

There were >180 commits in the branch to be reviewed. I reduced this list somewhat by using this command:

    last_release="29.4"

    # Exclude commits that *only* touch translations, tests, gpg keys, and
    # team internal tooling.
    git log pkg/import/${last_release}.. -- . \
        ':(exclude)debian/po' \
        ':(exclude)features' \
        ':(exclude)keyrings' \
        ':(exclude)tools' \
        ':(exclude)uaclient/cli/tests' \
        ':(exclude)uaclient/util.py'

I understand Robie used a similar approach, and he may have a more extensive set of excludes than me. This would be a good thing to add to the review docs for future reviewers. In my case, for R30.0 this reduced the number of commits by only 30. Despite that, I think this could be a good approach to help narrow the scope of work for review; as it was it ate the better part of 2 days just to glance through the 150 commits to find the ones needing deeper review. Being able to further filter the list of items requiring our review would be super helpful.

Thanks again for the review, Bryce.
I will be working on the FIXME items, and posting answers here when that's the case.

There is a new (small) set of commits to add to this branch, most tests and fixes for the purge things. I will add everything before asking for a new review.

Added 9 new commits adressing test changes and fixes related to those tests.

Added 14 commits addressing Bryce's 'Needs Fixing' review.
Besides that, I have a reply to the pastebin with some answers and excuses for those changes added here:

https://paste.ubuntu.com/p/mjgXyZZnw5/

5f0434a6b57825464591e6892c4a4ba7b0c538f2
  LGTM, +1

f126a05dcfaf3fb39526675be07edae2f52a0063
  LGTM, +1

67f8d1c4f8219d816b70b2847ab2f26a0b86a68b
  "test(aws): include deprecated aws pro fips images (CPC-3030)"
  Verified that pycloudlib 5.6.0 updates the api to add the include_deprecated param
  https://github.com/canonical/pycloudlib/releases/tag/1!5.6.0
  https://github.com/canonical/pycloudlib/pull/320/commits/3b13a3ace4871ea5fc5c702e65a3ec57f9cc1965

I don't fully understand the original problem that this is solving, or if there might be any side effects from the fix. The original bug report is terse, is there another discussion somewhere that could be referenced?

Moreover, if I understand the pycloudlib commit correctly the include_deprecated parameter only applies to the EC2 Cloud Class and classes derived from it, but here in uatools it's being used in uatool's Cloud base class which I assume is intended to work not just with EC2 clouds but others as well? Is that true, or can you help clear my misunderstanding?

Test case coverage would be nice to have as well, if at all possible...

bd8089d7f06661bf1fb457881778f2eedef71493
  "apt: change algorithm to look for valid versions"

This looks like a good refactoring, particularly in that it eliminates the extra looping for exclude_origin.

The first paragraph of the commit message is a bit confusing, and I don't quite understand the condition that leads to the issue, however shouldn't whatever that was also have a corresponding check added in the test cases?

I notice that one of the changes is an added check for `file.component != "now"`, which may be a bit obscure (I didn't know it offhand, at least), so may be worth a comment in either the code or commit message. I don't know if there is an Apt reference doc explaining it, but https://askubuntu.com/questions/892572/what-does-a-component-or-archive-now-mean-in-the-apt-python-api has a sensible answer.

4a73dd2a4fe4f56c029ccb0b2f148bdcca2bf9a3
  LGTM, +1

0e5d48afbbc2bb3aca1b405258818e8c2c8efbbc
  LGTM, +1

  The commit messages Real-time kernel as an exception but I don't see anything about that in the code; hopefully that doesn't indicate anything is missing?

fdc278965eb7980fbfd9f5ca27d7ee130b4b1568
  LGTM, +1
  Great to see the extra tests for the kernel purge case!

7dde9c533c07fba7a896c974ef85e876cc7ef032
  po update, +1

ee1e66de621ad98814f6e4be741e38fa67196fff
  tests ftw, +1

#: ../../uaclient/messages/__init__.py:697
#, python-brace-format
msgid ""
"This machine is receiving security patching for Ubuntu Main/Restricted\n"
"repository until {date}."
msgstr ""
"Esta máquina está recebendo patches de segurança para o repositório\n"
"Main/Restricted do Ubuntu até"

The pt_BR translation is missing the "{date}." variable.

There are a few fuzzy pt_BR translations still:
$ grep fuzzy -r debian/po/ | grep -v \\.pot
debian/po/pt_BR.po:#, fuzzy
debian/po/pt_BR.po:#, fuzzy
debian/po/pt_BR.po:#, fuzzy, python-brace-format
debian/po/pt_BR.po:#, fuzzy
debian/po/pt_BR.po:#, fuzzy, python-brace-format
debian/po/pt_BR.po:#, fuzzy, python-brace-format
debian/po/pt_BR.po:#, fuzzy, python-brace-format
debian/po/pt_BR.po:#, fuzzy

We talked about this already, but just so we don't forget (as there won't be bionic-specific MPs here in LP):

bionic doesn't support systemd's type exec:

/lib/systemd/system/ubuntu-advantage.service:27: Failed to parse service type, ignoring: exec

I filed https://github.com/canonical/ubuntu-pro-client/issues/2805 about an issue with `pro disable fips --purge`. I know --purge is marked as experimental, so I'll leave it up to you to decide how to handle it.

Also a reminder about https://github.com/canonical/ubuntu-pro-client/pull/2804, if you would like to have that in the release too.

misaligned columns in translated status output: https://github.com/canonical/ubuntu-pro-client/issues/2806

unattached status output has untranslated pt_BR strings:

# pro status
SERVIÇO DISPONÍVEL DESCRIÇÃO
anbox-cloud yes Android escalável na nuvem
esm-apps yes Expanded Security Maintenance for Applications
esm-infra yes Expanded Security Maintenance for Infrastructure
livepatch yes Canonical Livepatch service
realtime-kernel yes Ubuntu kernel with PREEMPT_RT patches integrated
usg yes Security compliance and audit tools

That "yes" needs to be handled carefully: there are places where it must not be translated I heard, like machine-parsable output (json).

There seem to be different encodings at play in the `pro status --format` output, at least for json and yaml.

json:
"execution_details": "Nenhuma opera\u00e7\u00e3o Ubuntu Pro est\u00e1 sendo executada"

yaml:
execution_details: "Nenhuma opera\xE7\xE3o Ubuntu Pro est\xE1 sendo executada"

Unsure if this is a problem. I quickly imported those into python using json and yaml, and nothing crashed at least.

On the unicode character encoding differences: we use python stdlib json, and pyyaml without any customization, so all should be fine. And I went ahead and checked the JSON and YAML specs and this is the correct encoding for each, respectively.

JSON requires the "\u1234" encoding even for codepoints < 0xFF. See "9 String" here: https://www.ecma-international.org/wp-content/uploads/ECMA-404_2nd_edition_december_2017.pdf

YAML allows different encoding for 8-bit, 16-bit and 32-bit codepoints. The 8-bit encoding looks like "\x12" and that is what is being used here. See "rule-ns-esc-8-bit" here: https://yaml.org/spec/1.2.2/#escaped-characters

@bryce:

> 67f8d1c4f8219d816b70b2847ab2f26a0b86a68b
> I don't fully understand the original problem that this is
> solving, or if there might be any side effects from the
> fix. The original bug report is terse, is there another
> discussion somewhere that could be referenced?

The referenced Jira ticket is the best we have
https://warthogs.atlassian.net/browse/CPC-3030

There should be no side effects: when testing, we include deprecated images in the search for the daily image.
That is pretty much it.
It happens because AWS is (incorrectly) setting some valid images as deprecated, and it was breaking us.
CPC is now aware and monitoring those things with AWS.

> Moreover, if I understand the pycloudlib commit correctly
> the include_deprecated parameter only applies to the EC2
> Cloud Class and classes derived from it, but here in
> uatools it's being used in uatool's Cloud base class which
> I assume is intended to work not just with EC2 clouds but
> others as well? Is that true, or can you help clear my
> misunderstanding?
Yes, this is currently only implemented on EC2, and could be implemented in the respective module... However, we only use this for our own test suite - we may change it if something breaks but it is unlikely.

> Test case coverage would be nice to have as well, if at all possible...
This is mostly a pycloudlib change that we adapted, and it is only part of our test suite - does not affect the package at all, so we don't have tests for these changes.

> 0e5d48afbbc2bb3aca1b405258818e8c2c8efbbc
> The commit messages Real-time kernel as an exception but
> I don't see anything about that in the code; hopefully
> that doesn't indicate anything is missing?

I double checked that, and supports_purge is set to False in Real-time kernel - so we should be good here, all set.

Added 21 more commits, addressing Andreas' review, and rounding up integration tests and PO file structure. This MP should be ready for another round now.

Thanks for adding the test case I flagged.

I still think you should leave some breadcrumbs for the deprecated images issue, since that seems like one of those weird glitchy corner cases that'll pop up some day, and a future engineer might appreciate pointers. And I also still think that using it from the base Cloud class will eventually cause problems. However, you're right this is only affecting test code, so irrelevant for the release as long as the test cases pass, and can be addressed/ignored at your discretion.

I don't spot anything in the new complaints to raise any concerns about.

+1

Thanks for the approval, Andreas!

We still need to coordinate uploads, because of pkgbinarymangler (https://bugs.launchpad.net/ubuntu/+source/pkgbinarymangler/+bug/2037584) - will get in touch soon so we can do it correctly

The two code changes and changelog entry LGTM, rest are just test updates.

This landed already. For some reason the bot didn't pick up the approval votes.

I'm marking this MP as merged to reflect the current status.