feat: register with content templates #168

rverdile · 2024-11-13T15:44:10Z

Card ID: CCT-769.

This is dependent on this PR, which the requirements are still being discussed, so I'm leaving this in draft for now.

This PR implements a new option to register with a content template, given the content template name. Since content templates are implemented as environments in candlepin, I've tried to implement this such that some code could be re-used in the future for registering with environments.

Feedback is appreciated and welcome :)

Still needs

To handle activation keys
To handle when GetEnvironments() is not available

Archana-PandeyM · 2024-11-14T10:14:54Z

/packit build

jirihnidek

Thanks for your PR. Overall, it works. 👍

I have to apologize. We agreed that we will support only environment with type == content-template. Thus, it will be possible to simplify your PR on some places. You can find more details in my comments.

I have some other requests.

jirihnidek · 2024-12-03T10:41:44Z

rhsm.go

+	if len(environments) > 0 && organization != "" {
+		var useEnvIDs = false
+		envList, err := getEnvironmentsList(privConn, username, password, organization)
+		if err != nil {


If err is not nil, then orgs with error should be returned. We will not allow to use content-templates on the systems, where D-Bus method GetEnvironments() is not available.

I agree this is a nice simplification. And now I'm thinking alternating to using the ID would not be useful in the case of content templates. @ptoscano wrote the ticket so he may want to provide some input here.

jirihnidek · 2024-12-03T10:52:35Z

rhsm.go

+	return environments, nil
+}
+
+func mapEnvironmentIDsToNames(names []string, environmentList []Environment, contentTemplates bool) ([]string, error) {


Few notes:

The name of this function is not correct. You try to get environment IDs from environment names. Thus the name should be something like mapEnvironmentNamesToIds() or getContentTemplateIds(). You return list (not map) of environments with type == "content-template". I vote for getContentTemplateIds().

This method is missing doc string with description.

Variable contentTemplates bool could be removed.

You can also name returned variables. Something like:

func getContentTemplateIds()(names []string, environmentList []Environment, contentTemplates bool) (envIDs []string, err error) { // Body of function

jirihnidek · 2024-12-03T10:53:31Z

rhsm.go

+			typeString := "environment"
+			if contentTemplates {
+				typeString = "content template"
+			}


This could be removed, because we will support only environments with type == "content-template"

jirihnidek · 2024-12-03T10:56:34Z

rhsm.go

+			if contentTemplates {
+				typeString = "content template"
+			}
+			return nil, fmt.Errorf(typeString+" named \"%s\" was not found", name)


The error should be modified and the logic of error detection should be also modified.

I claim that we should return error if any of provided environment is not found or the type is not "content-type".

jirihnidek · 2024-12-03T10:56:57Z

rhsm.go

+	return environments, nil
+}
+
+func getEnvironmentsList(conn *dbus.Conn, username, password, organization string) ([]Environment, error) {


Doc string is missing of this function.

jirihnidek · 2024-12-03T10:57:13Z

rhsm.go

+	return e.Type == "content-template"
+}
+
+func unpackEnvironments(s string) ([]Environment, error) {


Doc string is missing.

rhsm.go

jirihnidek · 2024-12-13T17:52:34Z

Following change in rhsm.service candlepin/subscription-manager#3480 would allow to simplify implementation of content template.

It will be possible to use environment names directly for both authentication methods
It will be possible to specify environment_type in argument of Register() and RegisterWithActivationKeys()
Thus, there will be no need to call GetEnvironments() D-Bus method and registration will not be blocked by another REST API call.

rverdile · 2024-12-17T19:39:48Z

Made the changes based on the new subscription manager PR!

jirihnidek · 2024-12-19T21:35:14Z

@rverdile I just merged candlepin/subscription-manager#3480 to main ... I will review your PR tomorrow.

jirihnidek · 2025-01-10T16:43:54Z

/packit build

jirihnidek

Thanks for the update 👍 . It looks good. We are almost there. Can you please rebase your PR? We started to use meson build system and your branch does not reflect it. I also have three small requests.

jirihnidek · 2025-01-10T21:13:34Z

rhsm.go

+	options := make(map[string]string)
+	if len(environments) != 0 {
+		options["environment_names"] = strings.Join(environments, ",")
+		options["environment_type"] = environmentType


I think that it is not necessary to pass environmentType as an argument to this function and you can use:

options["environment_type"] = EnvTypeContentTemplate

because we will use only "content-template".

jirihnidek · 2025-01-10T21:14:05Z

rhsm.go

+	options := make(map[string]string)
+	if len(environments) != 0 {
+		options["environment_names"] = strings.Join(environments, ",")
+		options["environment_type"] = environmentType


I think that it is not necessary to pass environmentType as an argument to this function and you can use:

options["environment_type"] = EnvTypeContentTemplate

because we will use only "content-template".

jirihnidek · 2025-01-10T21:15:03Z

.gitignore

@@ -7,3 +7,4 @@ BUILDROOT
 RPMS
 SOURCES
 SRPMS
+.idea


Please do not add this line to .gitignore as part of this PR. Such change is not related to this PR.

* card ID: CCT-769 * register to templates with --content-template <name>

rverdile · 2025-01-13T15:16:51Z

updated, thanks!

jirihnidek · 2025-01-14T10:11:53Z

/packit build

jirihnidek

LGTM

rverdile force-pushed the content-templates branch from ef842be to d443851 Compare November 18, 2024 19:02

rverdile marked this pull request as ready for review December 2, 2024 13:49

jirihnidek requested changes Dec 3, 2024

View reviewed changes

rverdile force-pushed the content-templates branch from d443851 to 100b773 Compare December 17, 2024 18:57

jirihnidek requested changes Jan 10, 2025

View reviewed changes

rverdile force-pushed the content-templates branch 3 times, most recently from 4b28a76 to 1884313 Compare January 13, 2025 15:13

feat: register with content templates

7447497

* card ID: CCT-769 * register to templates with --content-template <name>

rverdile force-pushed the content-templates branch from 1884313 to 7447497 Compare January 13, 2025 15:14

jirihnidek merged commit 3e54b9a into RedHatInsights:main Jan 14, 2025
18 of 22 checks passed

jirihnidek reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: register with content templates #168

feat: register with content templates #168

rverdile commented Nov 13, 2024 •

edited

Loading

Archana-PandeyM commented Nov 14, 2024

jirihnidek left a comment

jirihnidek Dec 3, 2024

rverdile Dec 3, 2024

jirihnidek Dec 3, 2024

jirihnidek Dec 3, 2024

jirihnidek Dec 3, 2024

jirihnidek Dec 3, 2024

jirihnidek Dec 3, 2024

jirihnidek commented Dec 13, 2024

rverdile commented Dec 17, 2024

jirihnidek commented Dec 19, 2024

jirihnidek commented Jan 10, 2025

jirihnidek left a comment

jirihnidek Jan 10, 2025

jirihnidek Jan 10, 2025

jirihnidek Jan 10, 2025

rverdile commented Jan 13, 2025

jirihnidek commented Jan 14, 2025

jirihnidek left a comment

@@ @@ -7,3 +7,4 @@ BUILDROOT @@
               RPMS
               SOURCES
               SRPMS
+              .idea

feat: register with content templates #168

feat: register with content templates #168

Conversation

rverdile commented Nov 13, 2024 • edited Loading

Archana-PandeyM commented Nov 14, 2024

jirihnidek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirihnidek commented Dec 13, 2024

rverdile commented Dec 17, 2024

jirihnidek commented Dec 19, 2024

jirihnidek commented Jan 10, 2025

jirihnidek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rverdile commented Jan 13, 2025

jirihnidek commented Jan 14, 2025

jirihnidek left a comment

Choose a reason for hiding this comment

rverdile commented Nov 13, 2024 •

edited

Loading