Closed lucasmoura closed 5 months ago
athos-ribeiro
commented
5 months ago Are the CI failures something we want to look at (perhaps you need to pin a different pyyaml version for those tests) ?
Anyway, this LGTM from a packaging point of view.
AFAIU, we are going to wait on an SRU pre-approval. When granted, I am going to build a source package from this branch and upload it. there will be no git-ubuntu involved here. Is this the intended path forward?
panlinux
commented
5 months ago I would also like to see the CI pass, or fixed. Ignoring it should be well explained, if we get to that.
panlinux
commented
5 months ago When I follow these steps below, I end up with a system that is attached, esm-infra is enabled, esm-infra-legacy is disabled, but the gpg key for esm-infra is removed:
ua attach <token>
# confirm just esm-infra (not legacy) is enabled
ua status
# confirm the esm key is there
apt-key list
# confirm both esm-infra and esm-infra-legacy are enabled
ua enable esm-infra-legacy
ua status
# confirm the same esm key from before is there
apt-key list
# confirm just esm-infra is enabled, and esm-infra-legacy is now disabled
ua disable esm-infra-legacy
ua status
# the esm key is gone
apt-key list
# it warns about the missing key and that it cannot authenticate esm-infra repositories anymore
apt-get update
# confirm screen from esm is available and without the negative pin, i.e., it can be installed
apt-cache policy screenI understand there is some weird interaction between esm-infra and esm-infra-legacy, but since esm-infra-legacy can be enabled on its own right after attach, it's weird that disabling esm-infra-legacy leads to the removal of the gpg key for ESM, and thus renders the ESM repository unsafe since no GPG checks are done anymore.
lucasmoura
commented
5 months ago @panlinux I have fixed the gpg issue
@panlinux @athos-ribeiro regarding travis CI, I have taken a look and handling all of these python dependencies now seems unfeasible and, even more important, I don't think the test result matter that much here. The tests that are failing here are:
py3-trusty: Not ideal, but we do run the unit tests during package build and everything is working as expected. We can also locally run pytest and everything is working as expected.
flake8-trusty: I was able to run that one locally, and this is the output it is providing:
/home/lucasmoura/projects/ubuntu-advantage-client/.tox/flake8-trusty/lib/python3.10/site-packages/pep8.py:99: FutureWarning: Possible nested set at position 1
EXTRANEOUS_WHITESPACE_REGEX = re.compile(r'[[({] | []}),;:]')
uaclient/tests/test_cli_status.py:28:80: E501 line too long (85 > 79 characters)
uaclient/tests/test_cli_status.py:29:80: E501 line too long (83 > 79 characters)
uaclient/tests/test_cli_status.py:30:80: E501 line too long (82 > 79 characters)
uaclient/tests/test_cli_status.py:31:80: E501 line too long (111 > 79 characters)
uaclient/tests/test_cli_status.py:33:80: E501 line too long (87 > 79 characters)We can see that this is only affecting a test file. And the issue is that we are trying to put a # noqa annotation on a multiline string and this version of flake8 is not properly handling that. Honestly, I don't think this is worth fixing.
mypy: I couldn't find any version of my mypy that is compatible with our current setup. However, since this change is not actually adding any new logic and we are not modifying any existing flow here, I cannot forsee mypy catching anything here.
black: Same issue as mypy, but since this is just codestyle and we have not added any new logic to the client, the only thing it might "fix" is the test files, which I don't think will bring that much gain here.
But please let me know if you disagree.
panlinux
commented
5 months ago All good on the flake/black/mypy. If the same set of unit tests is run at build time, also good there. If not, then a manual run also works for me.
lucasmoura
commented
5 months ago @panlinux I think the tests ran during package build should be the same. I have looked at the output of sbuild and I am seeing:
python3 -m pytest
============================= test session starts ==============================
platform linux -- Python 3.4.3 -- pytest-2.5.1
collected 487 items
test-requirements.txt .
uaclient/entitlements/tests/test_base.py .................................
uaclient/entitlements/tests/test_cc.py .......
uaclient/entitlements/tests/test_cis.py ..
uaclient/entitlements/tests/test_esm.py .....
uaclient/entitlements/tests/test_fips.py ..........................................................................................
uaclient/entitlements/tests/test_livepatch.py ......................................
uaclient/entitlements/tests/test_repo.py ......................
uaclient/tests/test_apt.py ..............................................................
uaclient/tests/test_cli.py ...........................
uaclient/tests/test_cli_attach.py ...........
uaclient/tests/test_cli_detach.py .........
uaclient/tests/test_cli_disable.py ......
uaclient/tests/test_cli_enable.py .........
uaclient/tests/test_cli_refresh.py ....
uaclient/tests/test_cli_status.py .......
uaclient/tests/test_config.py .......................................................
uaclient/tests/test_contract.py .............
uaclient/tests/test_gpg.py ..
uaclient/tests/test_serviceclient.py ...
uaclient/tests/test_status.py ..................
uaclient/tests/test_util.py ............................................................... Which is consistent to what I see running pytest locally (Even though, locally, we only detect 486 test cases :upside_down_face: )
athos-ribeiro
commented
5 months ago LGTM.
Since this is not a real PR but a review request comparing the 19.6 to the 19.7 branch, I suppose we are ready for an upload from the source branch here, right?
panlinux
commented
5 months ago This may be redundant, and it's been a while since I was a sponsor for u-a-t, but we may want a PR in launchpad against the actual package branch that is in trusty (LP being the source of truth).
lucasmoura
commented
5 months ago @panlinux I have compared the base branch here, 19.6-trsuty with the launchpad branch ubuntu/trusty-devel and the diff is minimal (.gitignore and a changelog date).
Due to that, and as we are trying to start cutting all of our releases on github (as can be seen here), I was under the assumption that we could use just this branch to cut the release.
But please let me know if you think that doesn't work. I can create the MP in launchpad without a problem
panlinux
commented
5 months ago Due to that, and as we are trying to start cutting all of our releases on github (as can be seen here), I was under the assumption that we could use just this branch to cut the release.
Whatever is easier for you. Just keep in mind that when reviewing what is actually uploaded to unapproved, the diff I will get will be against what is in the Ubuntu Archive, and that is what will matter, because that's what our users will get.
athos-ribeiro
commented
5 months ago I have compared the base branch here, 19.6-trsuty with the launchpad branch ubuntu/trusty-devel and the diff is minimal (.gitignore and a changelog date).
I suppose we do not want to change the changelog date for an already released version, right?
panlinux
commented
5 months ago I have compared the base branch here, 19.6-trsuty with the launchpad branch ubuntu/trusty-devel and the diff is minimal (.gitignore and a changelog date).
I suppose we do not want to change the changelog date for an already released version, right?
Yeah, that sounds unecessary, unless it's blatantly incorrect.
lucasmoura
commented
5 months ago I am closing this PR as we have already released this package
Why is this needed?
This PR adds support to the new esm-infra-legacy service on Trusty.
Test Steps
esm-infra-legacyon status, but the service is not enabledChecklist
Does this PR require extra reviews?