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

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

Merged
merged 11 commits into from
Sep 12, 2023
Merged
95 changes: 89 additions & 6 deletions cmd/ocm/create/cluster/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package cluster
import (
"encoding/json"
"fmt"
"github.com/openshift-online/ocm-cli/pkg/billing"
Copy link
Contributor

Choose a reason for hiding this comment

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

order of imports...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

"io"
"net"
"os"
Expand Down Expand Up @@ -68,6 +69,7 @@ var args struct {
clusterWideProxy c.ClusterWideProxy
gcpServiceAccountFile arguments.FilePath
etcdEncryption bool
subscriptionType string

// Scaling options
computeMachineType string
Expand All @@ -92,6 +94,8 @@ const clusterNameHelp = "will be used when generating a sub-domain for your clus

const subnetTemplate = "%s (%s)"

const subscriptionTypeTemplate = "%s (%s)"

// Creates a subnet options using a predefined template.
func setSubnetOption(subnet, zone string) string {
return fmt.Sprintf(subnetTemplate, subnet, zone)
Expand All @@ -102,6 +106,14 @@ func parseSubnet(subnetOption string) string {
return strings.Split(subnetOption, " ")[0]
}

func setSubscriptionTypeOption(id, description string) string {
return fmt.Sprintf(subscriptionTypeTemplate, id, description)
}

func parseSubscriptionType(subscriptionTypeOption string) string {
return strings.Split(subscriptionTypeOption, " ")[0]
}

// Cmd Constant:
var Cmd = &cobra.Command{
Use: "cluster [flags] NAME",
Expand Down Expand Up @@ -301,6 +313,16 @@ func init() {
fmt.Sprintf("Namespace Ownership Policy for ingress. Options are %s",
strings.Join(ingress.ValidNamespaceOwnershipPolicies, ",")),
)

fs.StringVar(
&args.subscriptionType,
"subscription-type",
billing.StandardSubscriptionType,
fmt.Sprintf("The subscription billing model for the cluster. Options are %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

missing punctuation mark

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strings.Join(billing.ValidSubscriptionTypes, ",")),
)
arguments.SetQuestion(fs, "subscription-type", "Subscription type:")
Cmd.RegisterFlagCompletionFunc("subscription-type", arguments.MakeCompleteFunc(getSubscriptionTypeOptions))
}

func osdProviderOptions(_ *sdk.Connection) ([]arguments.Option, error) {
Expand Down Expand Up @@ -377,6 +399,36 @@ func getVersionOptionsWithDefault(connection *sdk.Connection, channelGroup strin
return
}

func getSubscriptionTypeOptions(connection *sdk.Connection) ([]arguments.Option, error) {
options := []arguments.Option{}
billingModels, err := billing.GetBillingModels(connection)
if err != nil {
return options, err
}
for _, billingModel := range billingModels {
option := subscriptionTypeOption(billingModel.ID(), billingModel.Description())
//Standard billing model should always be the first option

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)
		}

if billingModel.ID() == billing.StandardSubscriptionType {
if len(options) == 0 { // nil or empty slice or after last element
options = append(options, option)
} else {
options = append(options[:1], options[0:]...)
options[0] = option
}
} else {
options = append(options, option)
}
}
return options, nil
}

func subscriptionTypeOption(id string, description string) arguments.Option {
option := arguments.Option{
Value: setSubscriptionTypeOption(id, description),
}
return option
}

func getMachineTypeOptions(connection *sdk.Connection) ([]arguments.Option, error) {
return provider.GetMachineTypeOptions(
connection.ClustersMgmt().V1(),
Expand Down Expand Up @@ -421,12 +473,29 @@ func preRun(cmd *cobra.Command, argv []string) error {
// Validate flags / ask for missing data.
fs := cmd.Flags()

// Get options for subscription type
subscriptionTypeOptions, _ := getSubscriptionTypeOptions(connection)
err = arguments.PromptOneOf(fs, "subscription-type", subscriptionTypeOptions)
if err != nil {
return err
}

// Only offer the 2 providers known to support OSD now;
// but don't validate if set, to not block `ocm` CLI from creating clusters on future providers.
providers, _ := osdProviderOptions(connection)
err = arguments.PromptOneOf(fs, "provider", providers)
if err != nil {
return err
// If marketplace-gcp subscription type is used, provider can only be GCP
gcpBillingModel, _ := billing.GetBillingModel(connection, billing.MarketplaceGcpSubscriptionType)
gcpSubscriptionTypeTemplate := subscriptionTypeOption(gcpBillingModel.ID(), gcpBillingModel.Description())
isGcpMarketplaceSubscriptionType := args.subscriptionType == gcpSubscriptionTypeTemplate.Value

if isGcpMarketplaceSubscriptionType {
fmt.Println("setting provider to", c.ProviderGCP)
args.provider = c.ProviderGCP
} else {
err = arguments.PromptOneOf(fs, "provider", providers)
if err != nil {
return err
}
}

if wasClusterWideProxyReceived() {
Expand All @@ -435,7 +504,12 @@ func preRun(cmd *cobra.Command, argv []string) error {
args.clusterWideProxy.Enabled = true
}

err = promptCCS(fs)
// If marketplace-gcp subscription type is used, ccs should by default be true
if isGcpMarketplaceSubscriptionType {
fmt.Println("setting ccs to 'true'")
args.ccs.Enabled = true
}
err = promptCCS(fs, args.ccs.Enabled == true)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

okay... I was not expecting that.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified

if err != nil {
return err
}
Expand Down Expand Up @@ -530,6 +604,7 @@ func preRun(cmd *cobra.Command, argv []string) error {
if err != nil {
return err
}

return nil
}

Expand All @@ -554,6 +629,10 @@ func run(cmd *cobra.Command, argv []string) error {
return err
}

if args.interactive {
args.subscriptionType = parseSubscriptionType(args.subscriptionType)
}

clusterConfig := c.Spec{
Name: args.clusterName,
Region: args.region,
Expand All @@ -577,6 +656,7 @@ func run(cmd *cobra.Command, argv []string) error {
Private: &args.private,
EtcdEncryption: args.etcdEncryption,
DefaultIngress: defaultIngress,
SubscriptionType: args.subscriptionType,
}

cluster, err := c.CreateCluster(connection.ClustersMgmt().V1(), clusterConfig, args.dryRun)
Expand Down Expand Up @@ -1036,8 +1116,11 @@ func promptExistingVPC(fs *pflag.FlagSet, connection *sdk.Connection) error {
return err
}

func promptCCS(fs *pflag.FlagSet) error {
err := arguments.PromptBool(fs, "ccs")
func promptCCS(fs *pflag.FlagSet, presetCCS bool) error {
var err error
if !presetCCS {
err = arguments.PromptBool(fs, "ccs")
}
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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.368
github.com/openshift/rosa v1.2.24
github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8
github.com/spf13/cobra v1.7.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,8 @@ github.com/onsi/gomega v1.17.0/go.mod h1:HnhC7FXeEQY45zxNK3PPoIUhzk/80Xly9PcubAl
github.com/onsi/gomega v1.19.0/go.mod h1:LY+I3pBVzYsTBU1AnDwOSxaYi9WoWiqgwooUqq9yPro=
github.com/onsi/gomega v1.27.8 h1:gegWiwZjBsf2DgiSbf5hpokZ98JVDMcWkUiigk6/KXc=
github.com/onsi/gomega v1.27.8/go.mod h1:2J8vzI/s+2shY9XHRApDkdgPo1TKT7P2u6fXeJKFnNQ=
github.com/openshift-online/ocm-sdk-go v0.1.356 h1:FFUAi+mR5pdBsp3pisXp7M4SiFdxKQwNwKTQNY7QDY0=
github.com/openshift-online/ocm-sdk-go v0.1.356/go.mod h1:KYOw8kAKAHyPrJcQoVR82CneQ4ofC02Na4cXXaTq4Nw=
github.com/openshift-online/ocm-sdk-go v0.1.368 h1:qP+gkChV8WDwwpkUw1xUyjTXKdvrwyd70Gff2GMUSeU=
github.com/openshift-online/ocm-sdk-go v0.1.368/go.mod h1:KYOw8kAKAHyPrJcQoVR82CneQ4ofC02Na4cXXaTq4Nw=
github.com/openshift/rosa v1.2.24 h1:vv0yYnWHx6CCPEAau/0rS54P2ksaf+uWXb1TQPWxiYE=
github.com/openshift/rosa v1.2.24/go.mod h1:MVXB27O3PF8WoOic23I03mmq6/9kVxpFx6FKyLMCyrQ=
github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU=
Expand Down
39 changes: 39 additions & 0 deletions pkg/billing/billing.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package billing

import (
sdk "github.com/openshift-online/ocm-sdk-go"
amv1 "github.com/openshift-online/ocm-sdk-go/accountsmgmt/v1"
)

const (
StandardSubscriptionType = "standard"
MarketplaceRhmSubscriptionType = "marketplace-rhm"
MarketplaceGcpSubscriptionType = "marketplace-gcp"
)

var ValidSubscriptionTypes = []string{StandardSubscriptionType, MarketplaceRhmSubscriptionType, MarketplaceGcpSubscriptionType}

func GetBillingModel(connection *sdk.Connection, billingModelID string) (*amv1.BillingModelItem, error) {
bilingModel, err := connection.AccountsMgmt().V1().BillingModels().BillingModel(billingModelID).Get().Send()
if err != nil {
return nil, err
}
return bilingModel.Body(), nil
}

func GetBillingModels(connection *sdk.Connection) ([]*amv1.BillingModelItem, error) {
response, err := connection.AccountsMgmt().V1().BillingModels().List().Send()
if err != nil {
return nil, err
}
billingModels := response.Items().Slice()
var validBillingModel []*amv1.BillingModelItem
for _, billingModel := range billingModels {
for _, validSubscriptionTypeId := range ValidSubscriptionTypes {
if billingModel.ID() == validSubscriptionTypeId {
validBillingModel = append(validBillingModel, billingModel)
}
}
}
return validBillingModel, nil
}
6 changes: 4 additions & 2 deletions pkg/cluster/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
)

Expand Down Expand Up @@ -68,6 +68,7 @@ type Spec struct {
ChannelGroup string
Expiration time.Time
EtcdEncryption bool
SubscriptionType string

// Scaling config
ComputeMachineType string
Expand Down Expand Up @@ -312,6 +313,7 @@ func CreateCluster(cmv1Client *cmv1.Client, config Spec, dryRun bool) (*cmv1.Clu
ID(config.Flavour),
).
EtcdEncryption(config.EtcdEncryption).
BillingModel(cmv1.BillingModel(config.SubscriptionType)).
Properties(clusterProperties)

clusterBuilder = clusterBuilder.Version(
Expand Down Expand Up @@ -703,7 +705,7 @@ func GetClusterAddOns(connection *sdk.Connection, clusterID string) ([]*AddOnIte
}

// Only display add-ons for which the org has quota
quotaCosts.Each(func(quotaCost *amsv1.QuotaCost) bool {
quotaCosts.Each(func(quotaCost *amv1.QuotaCost) bool {
relatedResources := quotaCost.RelatedResources()
for _, relatedResource := range relatedResources {
if relatedResource.ResourceType() == "add-on" &&
Expand Down
2 changes: 2 additions & 0 deletions pkg/cluster/describe.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ func PrintClusterDescription(connection *sdk.Connection, cluster *cmv1.Cluster)
"Infra: %d\n"+
"Computes: %s\n"+
"Product: %s\n"+
"Subscription type: %s\n"+
"Provider: %s\n"+
"Version: %s\n"+
"Region: %s\n"+
Expand All @@ -221,6 +222,7 @@ func PrintClusterDescription(connection *sdk.Connection, cluster *cmv1.Cluster)
cluster.Nodes().Infra(),
computesStr,
cluster.Product().ID(),
cluster.BillingModel(),
cluster.CloudProvider().ID(),
cluster.OpenshiftVersion(),
cluster.Region().ID(),
Expand Down