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

azure tests: move to fedora 37 #6546

Closed
wants to merge 33 commits into from
Closed

Conversation

flo-renaud
Copy link
Contributor

Signed-off-by: Florence Blanc-Renaud flo@redhat.com

@flo-renaud flo-renaud changed the title azure tests: move to fedora 37 [WIP] azure tests: move to fedora 37 Nov 18, 2022
@flo-renaud flo-renaud force-pushed the azuref37 branch 6 times, most recently from 9936a69 to a6c1b19 Compare November 22, 2022 17:02
@flo-renaud
Copy link
Contributor Author

I'm failing to understand why pylint and tox report a cyclic-import error:

ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipaplatform._importhook -> ipaplatform.osinfo -> ipaplatform.tasks))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipalib -> ipalib.frontend))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipaclient.plugins.rpcclient -> ipalib.rpc -> ipalib -> ipaclient.remote_plugins))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipaclient.frontend -> ipalib -> ipaclient.remote_plugins -> ipaclient.remote_plugins.compat))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipalib -> ipalib.crud -> ipalib.frontend))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipaserver.install.installutils -> ipaserver.install.service -> ipaserver.install.ldapupdate))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipaserver.install.installutils -> ipaserver.install.service -> ipaserver.install.ldapupdate -> ipaserver.install.replication))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipaclient.plugins.rpcclient -> ipalib -> ipaclient.remote_plugins))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipaclient.frontend -> ipalib -> ipaclient.remote_plugins -> ipaclient.remote_plugins.schema))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipaclient.remote_plugins -> ipaclient.remote_plugins.schema -> ipalib.frontend -> ipalib))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipatests.pytest_ipa.integration.resolver -> ipatests.pytest_ipa.integration.tasks))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipatests.pytest_ipa.integration.config -> ipatests.pytest_ipa.integration.host -> ipatests.pytest_ipa.integration.env_config))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipatests.pytest_ipa.integration.config -> ipatests.pytest_ipa.integration.env_config))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipatests.pytest_ipa.integration.firewall -> ipatests.pytest_ipa.integration.tasks))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipatests.pytest_ipa.integration.host -> ipatests.pytest_ipa.integration.resolver -> ipatests.pytest_ipa.integration.tasks))
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipatests.pytest_ipa.integration.config -> ipatests.pytest_ipa.integration.host -> ipatests.pytest_ipa.integration.resolver -> ipatests.pytest_ipa.integration.tasks -> ipatests.pytest_ipa.integration.env_config))

and

lib/python3.11/site-packages/ipapython/session_storage.py:1: [R0401(cyclic-import), ] Cyclic import (ipalib -> ipalib.frontend))
lib/python3.11/site-packages/ipapython/session_storage.py:1: [R0401(cyclic-import), ] Cyclic import (ipaclient.frontend -> ipalib -> ipaclient.remote_plugins -> ipaclient.remote_plugins.schema))
lib/python3.11/site-packages/ipapython/session_storage.py:1: [R0401(cyclic-import), ] Cyclic import (ipaclient.plugins.rpcclient -> ipalib.rpc -> ipalib -> ipaclient.remote_plugins))
lib/python3.11/site-packages/ipapython/session_storage.py:1: [R0401(cyclic-import), ] Cyclic import (ipaclient.remote_plugins -> ipaclient.remote_plugins.compat -> ipalib.frontend -> ipalib))
lib/python3.11/site-packages/ipapython/session_storage.py:1: [R0401(cyclic-import), ] Cyclic import (ipaclient.remote_plugins -> ipaclient.remote_plugins.schema -> ipalib.frontend -> ipalib))
lib/python3.11/site-packages/ipapython/session_storage.py:1: [R0401(cyclic-import), ] Cyclic import (ipaclient.frontend -> ipalib -> ipaclient.remote_plugins -> ipaclient.remote_plugins.compat))
lib/python3.11/site-packages/ipapython/session_storage.py:1: [R0401(cyclic-import), ] Cyclic import (ipalib -> ipalib.crud -> ipalib.frontend))

Any idea would be welcome, otherwise I will simply disable the check on the files ipatests/prci_definitions/prci_checker.py and ipapython/session_storage.py

@stanislavlevin
Copy link
Contributor

I can't reproduce this on Python3.10. Please, give me a bit more time to check it against Python3.11.

@stanislavlevin
Copy link
Contributor

Disablement of pylint on reported files wouldn't help because of incorrect(confusing?) reports.
Let's look at report
ipatests/prci_definitions/prci_checker.py:1: [R0401(cyclic-import), ] Cyclic import (ipaplatform._importhook -> ipaplatform.osinfo -> ipaplatform.tasks)).
This can be reproduced with:
python3 -m pylint -d all -e cyclic-import ipaplatform.
The report literally means that the import of ipaplatform._importhook may (or may not) lead to import error (due to a circular import).
To skip this warning:

--- a/ipaplatform/osinfo.py
+++ b/ipaplatform/osinfo.py
@@ -218,7 +218,7 @@ class OSInfo(Mapping):
     def container(self):
         if self._container is not None:
             return self._container
-        from ipaplatform.tasks import tasks
+        from ipaplatform.tasks import tasks  # pylint: disable=cyclic-import
         try:
             self._container = tasks.detect_container()
         except NotImplementedError:

Though I don't know why it's fired on Python3.11 (if it's related at all).

@stanislavlevin
Copy link
Contributor

Ok, it's not related to Python3.11 at all.
Exactly the same errors can be observed on Python3.10 with pylint in the single process mode.
We are running pylint in auto process mode (jobs=0 option in pylintrc) that should automatically detect number of cpu.
Pylint < 2.14.0 relies only on os.sched_getaffinity to get that number, while pylint 2.14.0 improved the logic for that (pylint-dev/pylint@1de6da1) and actually gets the value based on /sys/fs/cgroup/cpu/cpu.shares. The default cpu.shares for Docker is 1024 (https://docs.docker.com/config/containers/resource_constraints/#configure-the-default-cfs-scheduler), that implies that pylint will always use single process in auto mode in such environments.
Though, it may be worth taking into account pylint-dev/pylint#3232.

@stanislavlevin
Copy link
Contributor

Awkward silence. @flo-renaud, would you expect that someone fixes the issue? Please let me know if I should fix it.

@flo-renaud
Copy link
Contributor Author

Hi @stanislavlevin

Awkward silence. @flo-renaud, would you expect that someone fixes the issue? Please let me know if I should fix it.

Sorry for the delay and lack of response, I was caught in other tasks :(
First of all, thanks for investigating and finding the root cause. If you have the bandwidth to fix the issue, sure, please go ahead (you can add commits to this PR or make a separate, as you prefer).

@stanislavlevin
Copy link
Contributor

I will check it today.

@flo-renaud
Copy link
Contributor Author

@stanislavlevin thanks for the additional commits, LGTM.
Would be great to have an additional reviewer as I authored the first part of the PR and don't want to self-ack my code.

@stanislavlevin
Copy link
Contributor

I wanted to replace deprecated cgi as well, but it can be done in another PR.
@flo-renaud, sure. I'll take a look on this week.

@flo-renaud flo-renaud changed the title [WIP] azure tests: move to fedora 37 azure tests: move to fedora 37 Dec 15, 2022
@stanislavlevin
Copy link
Contributor

Still working on this.

@stanislavlevin
Copy link
Contributor

@flo-renaud, I added more commits addressing issues found during my review.
It's ready for review.

@flo-renaud
Copy link
Contributor Author

@stanislavlevin Thanks for your help on this one! I'm on vacation but will review as soon as I return early January.

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
pylint fixed issue pylint-dev/pylint#4756
and we don't need anymore to disable this check.

Related: https://pagure.io/freeipa/issue/9278
Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
The newer version of pylint has fixed false positives and
does not need anymore these suppressions:
- global-variable-not-assigned
- invalid-sequence-index
- no-name-in-module
- not-callable
- unsupported-assignment-operation

Related: https://pagure.io/freeipa/issue/9278
Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
flo-renaud and others added 20 commits January 2, 2023 18:49
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
Related: https://pagure.io/freeipa/issue/9278

Signed-off-by: Florence Blanc-Renaud <flo@redhat.com>
There are several known problems with multiprocess mode.
For example, pylint-dev/pylint#3232.

In other words the lint result depends on the number of jobs.
The most correct report is expected for single process.

Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
Fixes:
```
[E0611(no-name-in-module), ] No name 'parse' in module 'lxml.etree'
[E0611(no-name-in-module), ] No name 'murmurhash3' in module 'pysss_murmur'
```

Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
`extension-pkg-whitelist` is deprecated in favour of
`extension-pkg-allow-list` since Pylint 2.7.3:
https://pylint.pycqa.org/en/latest/whatsnew/2/2.7/full.html#what-s-new-in-pylint-2-7-3

Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
Most of `cyclic-import` issues reported by Pylint are false-positive
and they are already handled in the code, but several ones are the
actual errors.

Fixes: https://pagure.io/freeipa/issue/9232
Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
`pipes` module is deprecated as of Python 3.11.
https://docs.python.org/3/library/pipes.html#module-pipes:
> Deprecated since version 3.11, will be removed in version 3.13: The
  pipes module is deprecated (see PEP 594 for details).

IPA code used only `quote` function from `pipes` that in turn is
the alias for `shlex.quote` since Python 3.3:
python/cpython@9bce311

Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
> Emitted when a local variable is accessed before its assignment took
place. Assignments in try blocks are assumed not to have occurred when
evaluating associated except/finally blocks. Assignments in except
blocks are assumed not to have occurred when evaluating statements
outside the block, except when the associated try block contains a
return statement.

Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
https://pylint.pycqa.org/en/latest/user_guide/messages/warning/modified-iterating-list.html:
> Emitted when items are added or removed to a list being iterated
through. Doing so can result in unexpected behaviour, that is why it is
preferred to use a copy of the list.

https://docs.python.org/3/tutorial/controlflow.html#for-statements:
> Code that modifies a collection while iterating over that same
collection can be tricky to get right. Instead, it is usually more
straight-forward to loop over a copy of the collection or to create a
new collection

Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
https://pylint.pycqa.org/en/latest/user_guide/messages/convention/unnecessary-lambda-assignment.html:
> Used when a lambda expression is assigned to variable rather than
defining a standard function with the "def" keyword.

https://peps.python.org/pep-0008/#programming-recommendations:
> Always use a def statement instead of an assignment statement that
binds a lambda expression directly to an identifier:
def f(x): return 2*x
f = lambda x: 2*x
The first form means that the name of the resulting function object is
specifically ‘f’ instead of the generic ‘<lambda>’. This is more useful
for tracebacks and string representations in general. The use of the
assignment statement eliminates the sole benefit a lambda expression can
offer over an explicit def statement (i.e. that it can be embedded
inside a larger expression)

Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
https://pylint.pycqa.org/en/latest/user_guide/messages/error/unhashable-member.html:
> Emitted when a dict key or set member is not hashable (i.e. doesn't
define __hash__ method).

https://docs.python.org/3/library/stdtypes.html#dict.update:
> Update the dictionary with the key/value pairs from other, overwriting
existing keys. Return None.
update() accepts either another dictionary object or an iterable of
key/value pairs (as tuples or other iterables of length two). If keyword
arguments are specified, the dictionary is then updated with those
key/value pairs: d.update(red=1, blue=2).

Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/useless-object-inheritance.html:
> Used when a class inherit from object, which under python3 is
implicit, hence can be safely removed from bases.

Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
https://docs.python.org/3/library/cgi.html#module-cgi:
> Deprecated since version 3.11, will be removed in version 3.13: The
cgi module is deprecated (see PEP 594 for details and alternatives).

Fixes: https://pagure.io/freeipa/issue/9278
Signed-off-by: Stanislav Levin <slev@altlinux.org>
@flo-renaud
Copy link
Contributor Author

@stanislavlevin your updates LGTM. I rebased onto the master branch to make sure that no additional lint issues are detected.
If you agree with my commits please add the ACK label and I will merge when PRCI completes.

@stanislavlevin
Copy link
Contributor

@flo-renaud, thank you very much for the review :)

@stanislavlevin stanislavlevin added the ack Pull Request approved, can be merged label Jan 9, 2023
@flo-renaud flo-renaud added ipa-4-10 Mark for backport to ipa 4.10 pushed Pull Request has already been pushed labels Jan 10, 2023
@flo-renaud
Copy link
Contributor Author

master:

  • 232b5a9 azure tests: move to fedora 37
  • cad0638 pylint: remove unneeded disable=unused-private-member
  • 1206729 pylint: remove useless suppression
  • a9c1c81 pylint: disable redefined-slots-in-subclass
  • 2011d1a pylint: disable used-before-assignment
  • d1f1612 pylint: replace deprecated distutils module
  • be7f0a6 pylint: disable modified-iterating-list
  • 8cd9ddf pylint: remove arguments-renamed warnings
  • d6d8319 pylint: disable using-constant-test
  • 0268857 pylint: disable unnecessary-dunder-call message
  • 18fd448 pylint: globally disable unnecessary-lambda-assignment message
  • 139038c pylint: disable missing-timeout message
  • 2268ef4 pylint: fix implicit-str-concat
  • 8e7e48d pylint: fix duplicate-value
  • 6518855 pylint: fix deprecated-class SafeConfigParser
  • 372a5dc pylint: disable invalid-sequence-index
  • 7915365 pylint: disable unhashable-member
  • 8fad897 pylint: globally disable useless-object-inheritance
  • fdd3dd2 pylint: fix consider-iterating-dictionary
  • 416c210 pylint: disable comparison-of-constants
  • a4102b9 pylint: fix comparison-of-constants
  • fa4b054 pylint: disable deprecated-module message
  • a1a3b90 pylint: Lint in single process mode
  • deaec9b pylint: More allowed C extensions
  • ccdc94b pylint: Replace deprecated extension-pkg-whitelist
  • 4352bd5 pylint: Fix cyclic-import
  • a8dd070 pylint: Replace deprecated pipes
  • 0e03315 pylint: Fix used-before-assignment
  • 24db4dc pylint: Fix modified-iterating-list
  • bf3083c pylint: Fix unnecessary-lambda-assignment
  • c523e85 pylint: Fix unhashable-member
  • b848054 pylint: Fix useless-object-inheritance
  • 691b5d2 pylint: Replace deprecated cgi module

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged ipa-4-10 Mark for backport to ipa 4.10 pushed Pull Request has already been pushed
Projects
None yet
2 participants