OCM-2966 : Feat : Added subscription_type parameter to create cluster command #531

tirthct · 2023-07-31T16:50:09Z

Changed

Added subscription_type parameter to create cluster command
Added validation to have one of the specific options for the command
Defaulted subscription_type to standard

Tested

Cluster should be created based on the subscription_type parameter passed in cli : tested

cmd/ocm/create/cluster/cmd.go

+	standardSubscriptionType                   = "standard"
+	marketplaceRhmSubscriptionType             = "marketplace-rhm"
+	marketplaceGcpSubscriptionType             = "marketplace-gcp"


tirthct · 2023-09-07T20:07:56Z

cmd/ocm/create/cluster/cmd.go

+		fmt.Sprintf("The subscription billing model for the cluster. Options are %s",
+			strings.Join(ValidSubscriptionTypes, ",")),


cristianoveiga · 2023-08-01T17:24:26Z

Let's update the PR name/commit description with the latest convention

…nteractive marketplace-gcp

… to get billing model

cristianoveiga · 2023-09-06T20:56:03Z

cmd/ocm/create/cluster/cmd.go

+	}
+	for _, billingModel := range billingModels {
+		options = append(options, arguments.Option{
+			Value:       fmt.Sprintf("%s (%s)", billingModel.Id(), billingModel.Description()),


Why do we need to add the description to the value?

Added it before since interactive mode has a ? option to show description, but seems it only shows descriptions of the argument, not the drop down. Removed the description

…t value for subscription type

cristianoveiga · 2023-09-08T15:34:27Z

cmd/ocm/create/cluster/cmd.go

+	return options, nil
+}
+
+func getSubscriptionTypeIdFromDescription(connection *sdk.Connection, value string) string {


I would suggest using the same approach used for subnets, see https://github.com/openshift-online/ocm-cli/blob/main/cmd/ocm/create/cluster/cmd.go#L885

Used similar methods as subnets

cristianoveiga · 2023-09-08T15:40:12Z

cmd/ocm/create/cluster/cmd.go

+	return ""
+}
+
+func getBillingModel(connection *sdk.Connection, billingModelID string) (*amv1.BillingModelItem, error) {


We can make this function reusable (as well as getBillingModels) by moving them to its own package under pkg/billing, for example.

This logic doesn't really belong to cmd/ocm/create/cluster/cmd.go

Moved to the billing package

cristianoveiga · 2023-09-08T15:47:11Z

cmd/ocm/create/cluster/cmd.go

+		//Standard billing model should always be the first option
+		if billingModel.Id() == StandardSubscriptionType {
+			standardOption := arguments.Option{
+				Value: fmt.Sprintf("%s (%s)", billingModel.Id(), billingModel.Description()),


This logic is duplicated and probably deserves its own function

Create a function and reused wherever duplicated

tirthct · 2023-09-10T22:23:28Z

cmd/ocm/create/cluster/cmd.go

+		return options, err
+	}
+	for _, billingModel := range billingModels {
+		//Standard billing model should always be the first option


Simplified by using a function to create the option and reusing outside the if else block

I think we can simplify further...

How about this:

if billingModel.ID() == billing.StandardSubscriptionType { options = append([]arguments.Option{option}, options...) } else { options = append(options, option) }

cristianoveiga · 2023-09-08T15:50:49Z

cmd/ocm/create/cluster/cmd.go

-		return err
+	// If marketplace-gcp subscription type is used, provider can only be GCP
+	gcpBillingModel, _ := getBillingModel(connection, marketplaceGcpSubscriptionType)
+	isGcpSubscriptionType := args.subscriptionType == fmt.Sprintf("%s (%s)", gcpBillingModel.Id(), gcpBillingModel.Description())


isGcpSubscriptionType not all GCP subscriptions falls into this. I would suggest naming it isGcpMarketplace instead.

cristianoveiga · 2023-09-08T15:52:51Z

go.mod

@@ -12,7 +12,7 @@ require (
 	github.com/nwidger/jsoncolor v0.3.2
 	github.com/onsi/ginkgo/v2 v2.11.0
 	github.com/onsi/gomega v1.27.8
-	github.com/openshift-online/ocm-sdk-go v0.1.356
+	github.com/openshift-online/ocm-sdk-go v0.1.367


We will need to wait the next version as we are changing the BillingModelItem from a struct to a class.

See https://github.com/openshift-online/ocm-api-model/pull/831/files

Used latest sdk version

cristianoveiga · 2023-09-08T15:54:18Z

cmd/ocm/create/cluster/cmd.go

+	if isGcpSubscriptionType {
+		fmt.Println("setting ccs to 'true'")
+		args.ccs.Enabled = true


We could move this logic outside this function

Moved outside this function and passed a value on whether to prompt for ccs or not

cristianoveiga

@tirthct PTAL

cristianoveiga · 2023-09-11T19:46:54Z

cmd/ocm/create/cluster/cmd.go

@@ -19,6 +19,7 @@ package cluster
 import (
 	"encoding/json"
 	"fmt"
+	"github.com/openshift-online/ocm-cli/pkg/billing"


order of imports...

cristianoveiga · 2023-09-11T20:31:02Z

cmd/ocm/create/cluster/cmd.go

+		fmt.Println("setting ccs to 'true'")
+		args.ccs.Enabled = true
+	}
+	err = promptCCS(fs, args.ccs.Enabled == true)


promptCCS(fs, args.ccs.Enabled == true) can go in a else block here, no ? This way we could avoid the extra parameter.

We still need to call promtCCS as "service account file" prompt logic is present over there. And that is based on whether args.ccs.Enables is true or false

okay... I was not expecting that.

Should we replace "args.ccs.Enabled == true" with args.ccs.Enabled ?

cristianoveiga · 2023-09-11T20:36:10Z

cmd/ocm/create/cluster/cmd.go

+		&args.subscriptionType,
+		"subscription-type",
+		billing.StandardSubscriptionType,
+		fmt.Sprintf("The subscription billing model for the cluster. Options are %s",


missing punctuation mark

Keeping the options description consistent with https://github.com/openshift-online/ocm-cli/blob/main/cmd/ocm/create/cluster/cmd.go#L294 and https://github.com/openshift-online/ocm-cli/blob/main/cmd/ocm/create/cluster/cmd.go#L301

tirthct · 2023-09-11T20:46:21Z

pkg/cluster/cluster.go

@@ -27,7 +27,7 @@ import (
 	"time"

 	sdk "github.com/openshift-online/ocm-sdk-go"
-	amsv1 "github.com/openshift-online/ocm-sdk-go/accountsmgmt/v1"
+	amv1 "github.com/openshift-online/ocm-sdk-go/accountsmgmt/v1"


I changed these to keep is consistent with CS (they have used cmv1 instead of cmsv1), but we can revert this back

…ways

cristianoveiga

LGTM

tirthct · 2023-09-12T01:29:11Z

@gdbranco can you PTAL?

Added subscription_type parameter to create cluster command

73e9766

cristianoveiga reviewed Aug 1, 2023

View reviewed changes

cmd/ocm/create/cluster/cmd.go Outdated

Comment on lines 49 to 51

standardSubscriptionType = "standard"

marketplaceRhmSubscriptionType = "marketplace-rhm"

marketplaceGcpSubscriptionType = "marketplace-gcp"

This comment was marked as resolved.

Sign in to view

cristianoveiga reviewed Aug 1, 2023

View reviewed changes

Fetched subscription type options from ocm-sdk and disabled ccs for i…

206f64e

…nteractive marketplace-gcp

tirthct changed the title ~~OCM-2966 : Added subscription_type parameter to create cluster command~~ OCM-2966 : Feat : Added subscription_type parameter to create cluster command Sep 6, 2023

Added comments and used specific billing model type function from sdk…

7d41647

… to get billing model

cristianoveiga reviewed Sep 6, 2023

View reviewed changes

Added prompt messages for default selection, set standard as the firs…

7881eb9

…t value for subscription type

cristianoveiga reviewed Sep 8, 2023

View reviewed changes

cristianoveiga requested changes Sep 8, 2023

View reviewed changes

tirthct added 3 commits September 10, 2023 18:22

Added separate billing package and simplified function logic

cbfc5f4

Modified subscription type logic to be consistent with subnet logic

c29ee4d

upgrade sdk version to 368

dd79832

cristianoveiga reviewed Sep 11, 2023

View reviewed changes

Fixed order of imports

92422d8

cristianoveiga reviewed Sep 11, 2023

View reviewed changes

tirthct added 2 commits September 11, 2023 16:55

simplified the logic to add standard billing model as first option al…

1b0df07

…ways

Simplified boolean condition

1bfbd0f

cristianoveiga approved these changes Sep 11, 2023

View reviewed changes

gdbranco approved these changes Sep 12, 2023

View reviewed changes

Fixed linter error

abe8d9c

gdbranco merged commit 59c8a6d into openshift-online:main Sep 12, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCM-2966 : Feat : Added subscription_type parameter to create cluster command #531

OCM-2966 : Feat : Added subscription_type parameter to create cluster command #531

tirthct commented Jul 31, 2023

This comment was marked as resolved.

This comment was marked as resolved.

tirthct Sep 7, 2023

cristianoveiga commented Aug 1, 2023

cristianoveiga Sep 6, 2023

tirthct Sep 7, 2023

cristianoveiga Sep 8, 2023

tirthct Sep 11, 2023

cristianoveiga Sep 8, 2023

tirthct Sep 10, 2023

cristianoveiga Sep 8, 2023

tirthct Sep 10, 2023

This comment was marked as resolved.

tirthct Sep 10, 2023

cristianoveiga Sep 11, 2023

cristianoveiga Sep 8, 2023

tirthct Sep 10, 2023

cristianoveiga Sep 8, 2023

tirthct Sep 11, 2023

cristianoveiga Sep 8, 2023

tirthct Sep 10, 2023

cristianoveiga left a comment

cristianoveiga Sep 11, 2023

tirthct Sep 11, 2023

cristianoveiga Sep 11, 2023

tirthct Sep 11, 2023

cristianoveiga Sep 11, 2023

tirthct Sep 11, 2023

cristianoveiga Sep 11, 2023

tirthct Sep 11, 2023

This comment was marked as resolved.

tirthct Sep 11, 2023

cristianoveiga left a comment

tirthct commented Sep 12, 2023

		fmt.Sprintf("The subscription billing model for the cluster. Options are %s",
		strings.Join(ValidSubscriptionTypes, ",")),

OCM-2966 : Feat : Added subscription_type parameter to create cluster command #531

OCM-2966 : Feat : Added subscription_type parameter to create cluster command #531

Conversation

tirthct commented Jul 31, 2023

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

cristianoveiga commented Aug 1, 2023

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

This comment was marked as resolved.

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

cristianoveiga 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

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

cristianoveiga left a comment

Choose a reason for hiding this comment

tirthct commented Sep 12, 2023