Fix omitting credentials when CLI has already been instantiated #14449

lhellebr · 2024-03-19T13:40:11Z

Problem Statement

Omitting credentials doesn't work when CLI has already been instantiated

Solution

If it's been instantiated, set its omitting attribute to True for all the classes

lhellebr · 2024-03-19T13:40:36Z

trigger: test-robottelo
pytest: tests/foreman/destructive/test_ldap_authentication.py::test_positive_negotiate_manual_with_autonegotiation_disabled

Satellite-QE · 2024-03-19T15:22:52Z

PRT Result

Build Number: 6124
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/destructive/test_ldap_authentication.py::test_positive_negotiate_manual_with_autonegotiation_disabled --external-logging
Test Result : ================= 2 passed, 335 warnings in 5737.68s (1:35:37) =================

jameerpathan111

Co-reviewed with @ogajduse

jameerpathan111 · 2024-03-20T10:38:59Z

robottelo/hosts.py

+                                getattr(self._cli, name).omitting_credentials = True
+                        except AttributeError:
+                            # not everything has an mro method, we don't care about them
+                            pass
        yield
        self.omitting_credentials = False


@lhellebr should we check self._cli._configured is configured in the teardown phase too? what happens if we don't?

I think if someone does

with parametrized_enrolled_sat.omit_credentials(): pass

Then CLI won't get instantiated at all (because parametrized_enrolled_sat.cli) hasn't been called. I haven't tested that though. In any case, I employed this just as a defensive programming technique to avoid error in cases we may not even think about.

ogajduse

co-reviewed with @jameerpathan111

ogajduse · 2024-03-20T10:32:27Z

robottelo/hosts.py

+            for file in Path('robottelo/cli/').iterdir():
+                if file.suffix == '.py' and not file.name.startswith('_'):
+                    cli_module = importlib.import_module(f'robottelo.cli.{file.stem}')
+                    for name, obj in cli_module.__dict__.items():
+                        try:
+                            if Base in obj.mro():
+                                getattr(self._cli, name).omitting_credentials = True
+                        except AttributeError:
+                            # not everything has an mro method, we don't care about them
+                            pass


Initially, @jameerpathan111 and I had some concerns about code repetition. But looking at it further, we concluded that it could be possibly remade into the following:

Suggested change

for file in Path('robottelo/cli/').iterdir():

if file.suffix == '.py' and not file.name.startswith('_'):

cli_module = importlib.import_module(f'robottelo.cli.{file.stem}')

for name, obj in cli_module.__dict__.items():

try:

if Base in obj.mro():

getattr(self._cli, name).omitting_credentials = True

except AttributeError:

# not everything has an mro method, we don't care about them

pass

for name, obj in self._cli.__dict__.items():

with contextlib.suppress(AttributeError): # not everything has an mro method, we don't care about them

if Base in obj.mro():

getattr(self._cli, name).omitting_credentials = True

With this change, it does not seem that much repetitive. 😃 We are not sure if it would work, though. But if the omit_credentials property assumes self._cli._configured, then we can rely on the classes being already populated there instead of going over robottelo/cli/.

I think cycling through robottelo/cli ensures we won't run into some attribute of cli that is for some reason descendant of Base - but it's not an entity we should instantiate cli for. I don't know if any such exists and more so, I don't know if any will exist in the future. I understand your point about repetition. Please consider what I just wrote and if you don't change your opinion, I can test your version.

The best person to address the

I don't know if any such exists and more so, I don't know if any will exist in the future.

part would be @JacobCallahan since he created the initial implementation of cli.

I do not see how your comment relates to the changes I suggested. The implementation of cli property ensures (through calling mro()) that only descendants of Base will be attributes of cli. If cli contains something that should not have been assigned as its attribute, then implementation of cli is the one to blame.

rplevka · 2024-04-05T15:29:01Z

robottelo/hosts.py

+                                getattr(self._cli, name).omitting_credentials = True
+                        except AttributeError:
+                            # not everything has an mro method, we don't care about them
+                            pass
        yield
        self.omitting_credentials = False


This Sat object attribute is not necessarily False every time. One can initialize Satellite with True (by explicitly setting it in kwargs). In such scenario, this wouldn't reset the attribute to the correct original version.
Perhaps this contextmanager function could check if the ommiting_credentials is set to False first, otherwise, it has no reason to run and could be skipped straight away.

lhellebr · 2024-04-15T15:30:52Z

trigger: test-robottelo
pytest: tests/foreman/destructive/test_ldap_authentication.py::test_positive_negotiate_manual_with_autonegotiation_disabled tests/foreman/destructive/test_ldap_authentication.py::test_positive_autonegotiate

Satellite-QE · 2024-04-15T18:11:34Z

PRT Result

Build Number: 6506
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/destructive/test_ldap_authentication.py::test_positive_negotiate_manual_with_autonegotiation_disabled tests/foreman/destructive/test_ldap_authentication.py::test_positive_autonegotiate --external-logging
Test Result : ================= 4 passed, 864 warnings in 9264.29s (2:34:24) =================

JacobCallahan

You can likely simplify the reversion by adding all modified objects into a list called modified, then iterate over that list post-yield to update omitting_credentials again.

JacobCallahan · 2024-04-16T20:55:50Z

robottelo/hosts.py

+                        AttributeError
+                    ):  # not everything has an mro method, we don't care about them
+                        if Base in obj.mro():
+                            getattr(self._cli, name).omitting_credentials = True


should this be True or False, if it's a reversion of the previous action?

lhellebr · 2024-04-23T08:57:07Z

trigger: test-robottelo
pytest: tests/foreman/destructive/test_ldap_authentication.py::test_positive_negotiate_manual_with_autonegotiation_disabled tests/foreman/destructive/test_ldap_authentication.py::test_positive_autonegotiate

Satellite-QE · 2024-04-23T12:06:37Z

PRT Result

Build Number: 6647
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/destructive/test_ldap_authentication.py::test_positive_negotiate_manual_with_autonegotiation_disabled tests/foreman/destructive/test_ldap_authentication.py::test_positive_autonegotiate --external-logging
Test Result : =========== 4 passed, 1006 warnings, 1 error in 10758.13s (2:59:18) ============

lhellebr · 2024-04-23T14:25:26Z

trigger: test-robottelo
pytest: tests/foreman/destructive/test_ldap_authentication.py::test_positive_negotiate_manual_with_autonegotiation_disabled tests/foreman/destructive/test_ldap_authentication.py::test_positive_autonegotiate

Satellite-QE · 2024-04-23T17:20:01Z

PRT Result

Build Number: 6658
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/destructive/test_ldap_authentication.py::test_positive_negotiate_manual_with_autonegotiation_disabled tests/foreman/destructive/test_ldap_authentication.py::test_positive_autonegotiate --external-logging
Test Result : ================ 4 passed, 986 warnings in 10078.11s (2:47:58) =================

JacobCallahan · 2024-04-23T20:54:52Z

@lhellebr if you need help with my recommended implementation, let me know.

lhellebr · 2024-04-24T12:35:42Z

Let's just merge. This PR is over a month old.

JacobCallahan · 2024-04-24T20:54:06Z

@lhellebr I don't see any priority labels on this PR, so would still like you to consider my suggestion. Here it is in code form:

    def omit_credentials(self):
        change = not self.omitting_credentials  # if not already set to omit
        changed = []
        if change:
            self.omitting_credentials = True
            # if CLI is already created
            if self._cli._configured:
                for name, obj in self._cli.__dict__.items():
                    with contextlib.suppress(
                        AttributeError
                    ):  # not everything has an mro method, we don't care about them
                        if Base in obj.mro():
                            attr = getattr(self._cli, name)
                            attr.omitting_credentials = True
                            changed.append(attr)
        yield
        if change:
            self.omitting_credentials = False
            for attr in changed:
                attr.omitting_credentials = False

* Fix omitting credentials when CLI has already been instantiated * Rework based on comments * Set to False after yield (cherry picked from commit 845ef03)

…ted (#14880) Fix omitting credentials when CLI has already been instantiated (#14449) * Fix omitting credentials when CLI has already been instantiated * Rework based on comments * Set to False after yield (cherry picked from commit 845ef03) Co-authored-by: Lukáš Hellebrandt <[email protected]>

…lliteQE#14449) * Fix omitting credentials when CLI has already been instantiated * Rework based on comments * Set to False after yield

Fix omitting credentials when CLI has already been instantiated

ce04003

lhellebr added CherryPick PR needs CherryPick to previous branches 6.15.z Introduced in or relating directly to Satellite 6.15 labels Mar 19, 2024

lhellebr self-assigned this Mar 19, 2024

lhellebr requested a review from a team as a code owner March 19, 2024 13:40

Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Mar 19, 2024

pondrejk approved these changes Mar 19, 2024

View reviewed changes

jameerpathan111 reviewed Mar 20, 2024

View reviewed changes

ogajduse reviewed Mar 20, 2024

View reviewed changes

rplevka requested changes Apr 5, 2024

View reviewed changes

Rework based on comments

0bd5010

Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Apr 15, 2024

Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Apr 15, 2024

lhellebr requested review from rplevka, ogajduse and jameerpathan111 April 16, 2024 07:07

JacobCallahan requested changes Apr 16, 2024

View reviewed changes

jameerpathan111 approved these changes Apr 19, 2024

View reviewed changes

Set to False after yield

b1959d0

Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Apr 23, 2024

Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Apr 23, 2024

Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Apr 23, 2024

Satellite-QE removed the PRT-Failed Indicates that latest PRT run is failed for the PR label Apr 23, 2024

lhellebr requested a review from JacobCallahan April 24, 2024 12:35

pondrejk merged commit 845ef03 into SatelliteQE:master Apr 25, 2024
13 checks passed

Satellite-QE mentioned this pull request Apr 25, 2024

[6.15.z] Fix omitting credentials when CLI has already been instantiated #14880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix omitting credentials when CLI has already been instantiated #14449

Fix omitting credentials when CLI has already been instantiated #14449

lhellebr commented Mar 19, 2024

lhellebr commented Mar 19, 2024

Satellite-QE commented Mar 19, 2024

jameerpathan111 left a comment

jameerpathan111 Mar 20, 2024

lhellebr Mar 21, 2024

ogajduse left a comment

ogajduse Mar 20, 2024

lhellebr Mar 21, 2024

lhellebr Mar 25, 2024

ogajduse Apr 5, 2024

rplevka Apr 5, 2024 •

edited

Loading

lhellebr commented Apr 15, 2024

Satellite-QE commented Apr 15, 2024

JacobCallahan left a comment

JacobCallahan Apr 16, 2024

lhellebr commented Apr 23, 2024

Satellite-QE commented Apr 23, 2024

lhellebr commented Apr 23, 2024

Satellite-QE commented Apr 23, 2024

JacobCallahan commented Apr 23, 2024

lhellebr commented Apr 24, 2024

JacobCallahan commented Apr 24, 2024

Fix omitting credentials when CLI has already been instantiated #14449

Fix omitting credentials when CLI has already been instantiated #14449

Conversation

lhellebr commented Mar 19, 2024

Problem Statement

Solution

lhellebr commented Mar 19, 2024

Satellite-QE commented Mar 19, 2024

jameerpathan111 left a comment

Choose a reason for hiding this comment

jameerpathan111 Mar 20, 2024

Choose a reason for hiding this comment

lhellebr Mar 21, 2024

Choose a reason for hiding this comment

ogajduse left a comment

Choose a reason for hiding this comment

ogajduse Mar 20, 2024

Choose a reason for hiding this comment

lhellebr Mar 21, 2024

Choose a reason for hiding this comment

lhellebr Mar 25, 2024

Choose a reason for hiding this comment

ogajduse Apr 5, 2024

Choose a reason for hiding this comment

rplevka Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

lhellebr commented Apr 15, 2024

Satellite-QE commented Apr 15, 2024

JacobCallahan left a comment

Choose a reason for hiding this comment

JacobCallahan Apr 16, 2024

Choose a reason for hiding this comment

lhellebr commented Apr 23, 2024

Satellite-QE commented Apr 23, 2024

lhellebr commented Apr 23, 2024

Satellite-QE commented Apr 23, 2024

JacobCallahan commented Apr 23, 2024

lhellebr commented Apr 24, 2024

JacobCallahan commented Apr 24, 2024

rplevka Apr 5, 2024 •

edited

Loading