feat: add GetEnvironments method to DBus register #3462

rverdile · 2024-10-01T19:41:30Z

Card-ID: CCT-764

This implements the functionality requested in the card, but I would like to get guidance on how to architect the implementation. Perhaps I should be implementing an environments class? The get_environments method I created makes multiple calls to the candlepin API, which I don't see precedence for looking at the other DBus methods.

rverdile · 2024-10-04T17:11:00Z

tested using this script, a modified example from: https://github.com/jirihnidek/rhsm-dbus-examples/tree/main

#!/bin/bash

source ./library.sh

start_register_server
export username="admin"
export password="admin"
export org_id="content-sources-test"



echo "Trying to get list of organizations using dbus-send..."
dbus-send --address=${my_addr} --print-reply --dest='com.redhat.RHSM1.Register' \
       '/com/redhat/RHSM1/Register' \
       com.redhat.RHSM1.Register.GetEnvironments string:${username} \
       string:${password} \
       string:${org_id} \
       dict:string:string:"","" \
       string:""


stop_register_server

ptoscano

Thanks for this PR, and sorry for the wait.

I added a couple of notes of things I noticed so far; will give a better try later on.

src/rhsmlib/dbus/objects/register.py

src/rhsm/connection.py

jirihnidek

Thanks for the PR. I have few requests.

src/rhsmlib/dbus/objects/register.py

jirihnidek · 2024-10-09T09:23:31Z

src/rhsmlib/dbus/objects/register.py

@@ -181,6 +181,32 @@ def get_organizations(self, options: dict) -> List[dict]:
        owners: List[dict] = uep.getOwnerList(options["username"])
        return owners

+    def get_environments(self, options: dict) -> List[dict]:


This method should not be in rhsmlib.dbus.objects.register module. This module should contain only methods related to D-Bus. This proposed method should be in new rhsmlib.services.environment module, because you can see that subscription-manager CLI already has environments sub-command. Thus, there will be potential to share new code.

This method should have optional argument uep (basic auth connection to candlepin) to be able to pass existing connection instance to this method.

I this method had another argument typed_environments: bool = True, then we could list not only typed environments. If typed_environments is False, then we could skip detection for that capability on server.

My understanding of the card is that the GetEnvironments() method is intended to be a DBus method. With that said, I moved the actual list call to candlepin to a newly created EnvironmentService class. I then instantiated that in the DBus method and used it to call to candlepin. Is that okay?

If I'm misunderstanding your request, I'm happy to make more changes :)

src/rhsmlib/dbus/objects/register.py

jirihnidek · 2024-10-09T13:02:09Z

test/rhsmlib/dbus/test_register.py

+    "type": "content-template"
+  }
+]
+"""


Please add more environments to this list. You will see that new unit test will start to fail.

I added more environments to this list (10 more), but I don't see an issue. If you're seeing something else, what else can I do to reproduce?

If the old code was used, then the unit test would fail.

rverdile · 2024-10-09T20:23:11Z

Thank you guys for the reviews! I addressed most of the feedback, and left some questions where I'm not sure.

My team usually squashes commits before merging, so I just added a new commit for the feedback. Please let me know if I should organize my commits differently :)

I also still need to work on the tests a bit and fix the linting

src/rhsmlib/dbus/objects/register.py

src/rhsmlib/services/environment.py

test/rhsmlib/dbus/test_register.py

rverdile · 2024-11-19T15:30:21Z

updated, thanks for the review!

ptoscano

Thanks for the updates! It seems to work fine, so I'm leaving a last round of miscellaneous minor changes to do the final polishing to this from my POV.

src/rhsmlib/services/environment.py

test/rhsmlib/services/test_environment.py

src/rhsmlib/dbus/objects/register.py

rverdile · 2024-11-21T13:47:21Z

updated, thanks!

ptoscano

Thanks for the changes! The code now LGTM, and it works well too.

Since @jirihnidek had notes/reviews, I'll approve leaving it to him for an additional review.

One small last thing from me (a forgotten previous comment from you, sorry):

My team usually squashes commits before merging, so I just added a new commit for the feedback. Please let me know if I should organize my commits differently :)

We generally don't squash commits on merging; would you please squash them in this PR, and provide a commit message that explains the changes done?

* Card-ID: CCT-764 * Method lists environments of the given org and returns a subset of the fields from candlepin * Provides a way to get the names of environments before registering in support of content templates

jirihnidek

LGTM, Thanks for updates

rverdile force-pushed the 764 branch 4 times, most recently from f438b2e to 3c6b79b Compare October 4, 2024 17:08

rverdile marked this pull request as ready for review October 4, 2024 17:11

ptoscano reviewed Oct 9, 2024

View reviewed changes

src/rhsmlib/dbus/objects/register.py Outdated Show resolved Hide resolved

src/rhsmlib/dbus/objects/register.py Outdated Show resolved Hide resolved

src/rhsm/connection.py Outdated Show resolved Hide resolved

jirihnidek requested changes Oct 9, 2024

View reviewed changes

rverdile force-pushed the 764 branch 2 times, most recently from 6926ca2 to af0d114 Compare October 9, 2024 20:12

rverdile mentioned this pull request Nov 13, 2024

feat: register with content templates RedHatInsights/rhc#168

Open

2 tasks

rverdile force-pushed the 764 branch from af0d114 to af645b7 Compare November 18, 2024 16:54

ptoscano requested changes Nov 19, 2024

View reviewed changes

src/rhsmlib/dbus/objects/register.py Outdated Show resolved Hide resolved

src/rhsmlib/dbus/objects/register.py Outdated Show resolved Hide resolved

src/rhsmlib/services/environment.py Outdated Show resolved Hide resolved

test/rhsmlib/dbus/test_register.py Outdated Show resolved Hide resolved

ptoscano requested changes Nov 20, 2024

View reviewed changes

rverdile force-pushed the 764 branch from d6c64aa to 06e4d18 Compare November 21, 2024 13:44

ptoscano approved these changes Nov 26, 2024

View reviewed changes

feat: add GetEnvironments method to DBus register

41d91d7

* Card-ID: CCT-764 * Method lists environments of the given org and returns a subset of the fields from candlepin * Provides a way to get the names of environments before registering in support of content templates

rverdile force-pushed the 764 branch from 06e4d18 to 41d91d7 Compare November 26, 2024 14:50

jirihnidek approved these changes Nov 27, 2024

View reviewed changes

jirihnidek merged commit 05870c4 into candlepin:main Nov 27, 2024
16 of 22 checks passed

jstavel mentioned this pull request Nov 28, 2024

get-environments added jirihnidek/rhsm-dbus-examples#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add GetEnvironments method to DBus register #3462

feat: add GetEnvironments method to DBus register #3462

rverdile commented Oct 1, 2024 •

edited

Loading

rverdile commented Oct 4, 2024

ptoscano left a comment

jirihnidek left a comment

jirihnidek Oct 9, 2024

rverdile Oct 9, 2024

rverdile Oct 9, 2024

jirihnidek Oct 9, 2024

rverdile Oct 9, 2024

jirihnidek Oct 11, 2024

rverdile commented Oct 9, 2024 •

edited

Loading

rverdile commented Nov 19, 2024

ptoscano left a comment

rverdile commented Nov 21, 2024

ptoscano left a comment

jirihnidek left a comment

feat: add GetEnvironments method to DBus register #3462

feat: add GetEnvironments method to DBus register #3462

Conversation

rverdile commented Oct 1, 2024 • edited Loading

rverdile commented Oct 4, 2024

ptoscano left a comment

Choose a reason for hiding this comment

jirihnidek left a comment

Choose a reason for hiding this comment

jirihnidek Oct 9, 2024

Choose a reason for hiding this comment

rverdile Oct 9, 2024

Choose a reason for hiding this comment

rverdile Oct 9, 2024

Choose a reason for hiding this comment

jirihnidek Oct 9, 2024

Choose a reason for hiding this comment

rverdile Oct 9, 2024

Choose a reason for hiding this comment

jirihnidek Oct 11, 2024

Choose a reason for hiding this comment

rverdile commented Oct 9, 2024 • edited Loading

rverdile commented Nov 19, 2024

ptoscano left a comment

Choose a reason for hiding this comment

rverdile commented Nov 21, 2024

ptoscano left a comment

Choose a reason for hiding this comment

jirihnidek left a comment

Choose a reason for hiding this comment

rverdile commented Oct 1, 2024 •

edited

Loading

rverdile commented Oct 9, 2024 •

edited

Loading