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

tools: adding dnf support #29

Merged
merged 1 commit into from
Aug 23, 2023
Merged

tools: adding dnf support #29

merged 1 commit into from
Aug 23, 2023

Conversation

danlavu
Copy link

@danlavu danlavu commented Aug 7, 2023

No description provided.

@danlavu danlavu requested a review from pbrezina August 7, 2023 14:02
@danlavu danlavu self-assigned this Aug 7, 2023
@ikerexxe ikerexxe assigned ikerexxe and aplopez and unassigned danlavu Aug 8, 2023
aplopez
aplopez previously approved these changes Aug 8, 2023
Copy link
Contributor

@aplopez aplopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@pbrezina
Copy link
Member

pbrezina commented Aug 8, 2023

Hi, why do you need dnf? All packages should be installed by sssd-ci-containers project. In an unlikely case that you need dnf for something very specific, you must make sure to revert changes done by dnf during teardown.

@danlavu
Copy link
Author

danlavu commented Aug 8, 2023

For the authselect tests, there is a test to ensure that the minimal profile doesn't require sssd-ad, sssd-ldap and other packages. How do you propose that we write that test? It is more of a functional system/build/maintainer test.

@ikerexxe
Copy link
Contributor

ikerexxe commented Aug 9, 2023

For the authselect tests, there is a test to ensure that the minimal profile doesn't require sssd-ad, sssd-ldap and other packages. How do you propose that we write that test? It is more of a functional system/build/maintainer test.

What's exactly the package that you need to install to test it?

@pbrezina
Copy link
Member

Me and Dan talked, one use case (removing the sssd packages) can be implemented in other way. Second use case is about removing authselect and verifying that authselect configuration was converted to non-authselect configuration which really requires the authselect package to be removed.

For this Dan needs dnf support, but the changes must be automatically reverted. Using dnf history undo will help here. Perhaps we can get current history ID when we first use dnf and then revert to this id on teardown.

@danlavu
Copy link
Author

danlavu commented Aug 16, 2023

For the authselect tests, there is a test to ensure that the minimal profile doesn't require sssd-ad, sssd-ldap and other packages. How do you propose that we write that test? It is more of a functional system/build/maintainer test.

What's exactly the package that you need to install to test it?

The code for this test case is tough to read. We will certainly improve it when porting the test suite over. As far as I can tell, sssd-client, sssd-common, sssd-nfs-idmap

       all_delete1 = [i for i in all_rpms
                       if (i.startswith('sssd-client') == False
                           and i.startswith('sssd-common') == False
                           and i.startswith('sssd-nfs-idmap') == False)]
        all_delete2 = [i for i in all_rpms if i.startswith('sssd-common-pac')]
        all_final = all_delete1 + all_delete2
        all_keep1 = [i for i in all_rpms if
                     (i.startswith('sssd-client') == True
                      or i.startswith('sssd-common') == True
                      or i.startswith('sssd-nfs-idmap') == True)]

@danlavu
Copy link
Author

danlavu commented Aug 16, 2023

@pbrezina @aplopez

Getting the latest transaction ID isn't sufficient. In the event that a package is already installed, the command exits without errors and the teardown proceeds to undo the wrong ID. So I added a statement to compare the IDs before adding the command to the teardown list.

ikerexxe
ikerexxe previously approved these changes Aug 17, 2023
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the patch!

Signed-off-by: Dan Lavu <[email protected]>
@danlavu
Copy link
Author

danlavu commented Aug 18, 2023

Added "-y" to the module so we don't have to pass it through each time.

Copy link
Contributor

@aplopez aplopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looked good; now it looks better. Thanks.

@ikerexxe ikerexxe merged commit 25aa578 into SSSD:master Aug 23, 2023
2 checks passed
@danlavu danlavu deleted the dnf branch April 17, 2024 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants