Code review comment for ~chad.smith/ubuntu/+source/update-notifier:xenial-esm-product-url-renaming

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Bug 1901627 now has proper test steps - that former need is fixed.

You also explained on the same bug why you need to update the translations (thanks).
But if it it is indeed a change for the same overall cause then instead of:

870 + * data/apt_check.py: Update UA Infra: ESM product name and doc url
871 + (LP: #1901627)
872 + - data/apt_check.py: Update name and URL
873 + - tests/test_motd.py: adapt unittests to match new behavior
874 + * po/*.po: translation files with intltool-update -r

it should be

870 + * data/apt_check.py: Update UA Infra: ESM product name and doc url
871 + (LP: #1901627)
872 + - data/apt_check.py: Update name and URL
873 + - tests/test_motd.py: adapt unittests to match new behavior
874 + - po/*.po: translation files with intltool-update -r

---

Furthermore we now have (in B&X) some changes to fix up autopkgtests.
Those are mixed again and should be grouped along an SRU purpose.
E.g. file a new bug for the autopkgtest issues (explain when they will trigger, maybe a link to an old fail, and that it will be sufficiently verified by the autopkgtests being good on the upload). Especially important on this bug is to outline why it will not have regression potential - e.g. "we are only fixing debian/test/ that isn't active on the users system and therefore has no regression impact". But if you do fix other things (e.g. you change data/apt_check.py) - then you need to elaborate on this bug why it is:
a) needed to SRU this (why wasn't it a problem before, ...)
b) what regression risks are
d) how to verify
I'd recommend to group all those changes in the cahngelog as well.
From:

[ Julian Andres Klode ]
* data/apt_check.py: Update package messaging
  "packages can be installed" -> "updates can be installed immediately"

[ Brian Murray ]
* data/apt_check & data/backend_helper: resolve underindent pep8 issues
  backport of 9e0f7ee50

[ Andrea Azzarone ]
* data/apt_check.py, data/package-data-downloader, tests/test_pep8.py:
  - update the code formating to be not hit W504 warnings,
    change to ignore W503 and be consistent with update-manager.

[ Gianfranco Costamagna ]
* INSTALL, data/hooks.py, tests/test_package-data-downloader.py:
  Fix E117 over-indented pep issues.

To maybe:

* Fix pep8 issues breaking autopkgtest NOW NEEDED BECAUSE OF TODO (LP: #BUG-TODO)
  - data/apt_check & data/backend_helper: resolve underindent pep8 issues
    backport of 9e0f7ee50 [ Brian Murray ]
  - data/apt_check.py, data/package-data-downloader, tests/test_pep8.py:
    - update the code formating to be not hit W504 warnings,
      change to ignore W503 and be consistent with update-manager.
    [ Andrea Azzarone ]
  - INSTALL, data/hooks.py, tests/test_package-data-downloader.py:
    Fix E117 over-indented pep issues. [ Gianfranco Costamagna ]

Again anyone looking at it will
a) see and be able to verify that all these changes belong to the scope of one "issue"
b) can check the bgu for details on reasoning, verification and potential impact

----

That finally leaves:

[ Julian Andres Klode ]
* data/apt_check.py: Update package messaging
  "packages can be installed" -> "updates can be installed immediately"

So again (like initially with the translation) - why is this change needed?
Is it part of the autopkgtest/pep8 fixes - if so why?

Better grouping and potentially explaining on the (to be created) bug will help.

review: Needs Fixing

« Back to merge proposal