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

Jump to bottom

Incorporate custodia into IPA #5831

Closed
wants to merge 7 commits into from
Closed

Incorporate custodia into IPA #5831

wants to merge 7 commits into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Jun 11, 2021 •
edited

@tiran tiran added WIP Work in progress - not ready yet for review ipa-next Mark as master (4.12) only labels Jun 11, 2021
@tiran tiran marked this pull request as ready for review June 11, 2021 10:54
@tiran tiran removed the WIP Work in progress - not ready yet for review label Jun 11, 2021
@tiran tiran requested a review from rcritten June 11, 2021 10:55
@abbra
Copy link
Contributor

abbra commented Jun 11, 2021

@tiran thanks for PR. Do we need to obsolete python3-custodia? Are you planing to remove it from the distributions too? Perhaps, obsoleting is not needed right now as we are not going to conflict by file paths anyway, right?

@tiran
Copy link
Member Author

tiran commented Jun 11, 2021

I changed the entry point names and import paths so our copy does not conflict with python3-custodia. Eventually we can drop custodia and python3-custodia from RHEL and Fedora. There is no need to rush, though.

@abbra
Copy link
Contributor

abbra commented Jun 11, 2021

Ok, next question is whether we should import this in ipa-4-9 branch. Without that I am not sure when this change would hit the distributions. 4.10? 5.0? How bad is it to include it into 4.9.5?

@tiran
Copy link
Member Author

tiran commented Jun 11, 2021

I don't see any harm to backport the change to 4.9. You may want to copy some unit tests from upstream into downstream first.

@rcritten
Copy link
Contributor

Backporting would let us accelerate the retirement of custodia as a separate package and retire it in rawhide and potentially downstream distributions.

@abbra
Copy link
Contributor

abbra commented Jun 11, 2021

Ok, may be we should then add those unit tests to IPA as well?

@abbra abbra added ipa-4-9 Mark for backport to ipa 4.9 and removed ipa-next Mark as master (4.12) only labels Jun 11, 2021
@rcritten
Copy link
Contributor

+1 on incorporating the tests

@rcritten
Copy link
Contributor

I filed upstream ticket to explain at least some of the reasoning behind this. Can you add this ticket to the commit msgs? https://pagure.io/freeipa/issue/8882

@tiran
Add Custodia 0.6.0 to ipaserver package
7c56d20 
Incorporate Custodia into IPA.

See: https://pagure.io/freeipa/issue/8882
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Remove unused Custodia modules
d134ff8 
The CLI, IPA integration and storage backends are not used by IPA.

See: https://pagure.io/freeipa/issue/8882
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Fix Custodia imports
8aba2a9 
See: https://pagure.io/freeipa/issue/8882
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Fix Custodia pylint issues
e410c87 
See: https://pagure.io/freeipa/issue/8882
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Remove more unused Custodia code
868b8d6 
See: https://pagure.io/freeipa/issue/8882
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran tiran force-pushed the custodia_merge branch 4 times, most recently from 784ffa1 to 9bc2b1c Compare June 14, 2021 13:59
@tiran
Add Custodia tests
5b02cbf 
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@rcritten
Copy link
Contributor

Take this with a grain of salt, I don't know custodia, but client.py and forwarder.py aren't used (except self-referrentially and client in one test).

Should we add a Provides so that python3-custodia and custodia can be removed on upgrade?

In general this change looks to be in good shape.

@tiran
Also drop Custodia client and forwarder
1cd2c3d 
See: https://pagure.io/freeipa/issue/8882
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Copy link
Member Author

tiran commented Jun 14, 2021

Take this with a grain of salt, I don't know custodia, but client.py and forwarder.py aren't used (except self-referrentially and client in one test).

Excellent finding! I removed the modules.

Should we add a Provides so that python3-custodia and custodia can be removed on upgrade?

The copied code does not provide python3-custodia and is independent from upstream Custodia. The import paths and plugin entry points are prefixed with ipaserver. You can have both python3-custodia package and freeipa-server package installed without any conflict.

I'll orphan custodia after the next update has landed in Fedora.

@rcritten
Copy link
Contributor

The only I asked about the Provides is on upgrades the custodia packages won't be removed. They don't harm anything per-se. Not a show stopper.

LGTM.

@rcritten rcritten added the ack Pull Request approved, can be merged label Jun 15, 2021
@rcritten
Copy link
Contributor

master:

  • 1e98f31 Add Custodia 0.6.0 to ipaserver package
  • d27f01b Remove unused Custodia modules
  • a4631b7 Fix Custodia imports
  • e1abfe0 Fix Custodia pylint issues
  • c27233e Remove more unused Custodia code
  • 470bb6e Add Custodia tests
  • e6f09c1 Also drop Custodia client and forwarder

@rcritten rcritten added the pushed Pull Request has already been pushed label Jun 16, 2021
@rcritten rcritten closed this Jun 16, 2021
@rcritten rcritten mentioned this pull request Jun 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcritten rcritten rcritten approved these changes

Assignees

@rcritten rcritten

Labels
ack Pull Request approved, can be merged ipa-4-9 Mark for backport to ipa 4.9 pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
3 participants
@tiran @abbra @rcritten