feat(kafka create): add billing model logic by rkpattnaik780 · Pull Request #1636 · redhat-developer/app-services-cli

rkpattnaik780 · 2022-07-12T10:00:47Z

This PR adds --billing-model flag to rhoas kafka create command.
The CLI should be able to create instances with standard, marketplace and trial quotas.

Verification Steps

For testing against marketplace (use _kafka_service account).

rhoas kafka create --name test-instance should throw error saying multiple quota types are available and user needs to specify billing model.
rhoas kafka create --name test-instance --billing-model standard should create a standard instance
rhoas kafka create --name test-instance --billing-model standard should create a marketplace instance

rhoas kafka create should start the creation in interactive mode and prompt for billing model.

User should be able to pick marketplace provider and market place account if multiple options are available, else the only account gets selected by default.

When only trial instances are available (use _kafka_devexp account)

Specifying the billing model should fail with message "only trial instances available".

rhoas kafka create --name test-instance should create a trial instance.

Interactive mode should not prompt billing model.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation change
Other (please specify)

wtrocki · 2022-07-12T10:09:30Z

pkg/cmd/kafka/create/create.go


-			if opts.bypassChecks && (opts.marketplace != "" || opts.marketplaceAcctId != "") {
+			if opts.bypassChecks && (opts.marketplace != "" || opts.marketplaceAcctId != "" || opts.billingModel != "") {
 				return f.Localizer.MustLocalizeError("kafka.create.error.bypassChecks.marketplace")


Description needs billing model flag

wtrocki · 2022-07-12T10:10:28Z

pkg/cmd/kafka/create/create.go

 				return f.Localizer.MustLocalizeError("kafka.create.error.bypassChecks.marketplace")
 			}

+			if opts.billingModel == "standard" && (opts.marketplaceAcctId != "" || opts.marketplace != "") {


I would suggest to not list any billing model in create.go. Also not hardcoding it's value.

wtrocki · 2022-07-12T10:11:52Z

pkg/cmd/kafka/create/create.go

 		return GetCloudProviderRegionCompletionValues(f, opts.provider)
 	})

-	_ = cmd.RegisterFlagCompletionFunc(FlagSize, func(cmd *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) {


Why removing those?

wtrocki · 2022-07-12T10:12:29Z

pkg/cmd/kafka/create/create.go

 		}

-		userInstanceType, err = accountmgmtutil.GetUserSupportedInstanceType(f.Context, &constants.Kafka.Ams, conn)
+		marketplaceInfo := accountmgmtutil.MarketplaceInfo{


Can be transformed as inline structure passed to method.

wtrocki · 2022-07-12T10:13:17Z

pkg/cmd/kafka/create/create.go

+		userInstanceType, err = accountmgmtutil.GetUserSupportedInstanceType(f, &constants.Kafka.Ams, marketplaceInfo)
 		if err != nil || userInstanceType == nil {
-			return f.Localizer.MustLocalizeError("kafka.create.error.userInstanceType.notFound")
+			return err // || f.Localizer.MustLocalizeError("kafka.create.error.userInstanceType.notFound")


userInstanceType == nil will print Error: nil to user

wtrocki · 2022-07-12T10:14:16Z

pkg/cmd/kafka/create/create.go


 	}

+	if userInstanceType.BillingModel == "standard" || userInstanceType.BillingModel == "marketplace" {


There are no other billing models and if they would be in future we should still pass them. I think this is redundant.

wtrocki · 2022-07-12T10:15:01Z

pkg/cmd/kafka/create/create.go

 		}
 	}

-	marketplaces, err := accountmgmtutil.GetValidMarketplaces(f.Context, f.Connection)


Why removing this?

If we want to ask user to pick marketplace details this should happen before size and feed back into size (this is current bug in relased version of the CLI). I do not see how interactive mode would work with marketplace right now. Would we fail/still require users to use flags for non interactive selects?

wtrocki · 2022-07-12T10:23:55Z

pkg/shared/accountmgmtutil/ams.go

 				} else if quotaResource.GetProduct() == spec.InstanceQuotaID {
 					remainingQuota := int(quota.GetAllowed() - quota.GetConsumed())
-					quotas = append(quotas, QuotaSpec{QuotaStandardType, remainingQuota, quotaResource.BillingModel})
+					if quotaResource.BillingModel == "standard" {


This seems redundant - standard will still have cloudAccounts with nil value.
It is never a bad idea to debug your code for understanding.

wtrocki · 2022-07-12T10:29:43Z

pkg/shared/accountmgmtutil/ams.go

+}

-	return amsType, nil
+type OrgQuotas struct {


While this is good I would myself keep all Quota spec as array and use filter flags (billingModel) to filter them out.

There is big decision that we can take:

Have trial as quota object (it will be always there) thus if logic is `len(quotas) == 1 { return quotas[0] }

Ignore trial and do not return quota for it (it will be more accurate from business logic point of view , but it will require rewrite of the create.go logic.

wtrocki · 2022-07-12T10:33:00Z

pkg/shared/accountmgmtutil/ams.go


+func SelectQuotaForUser(f *factory.Factory, orgQuota *OrgQuotas, marketplaceInfo MarketplaceInfo) (*QuotaSpec, error) {
+	if len(orgQuota.StandardQuotas) == 0 && len(orgQuota.MarketplaceQuotas) == 0 {
+		if len(orgQuota.TrialQuotas) == 0 {


Trial would be always present. Also doesn't need to checked in quota selection.

Decision we need to take here is:

do we want to fail when user specify rubbish billing model or accoutID that doesn't exist?

Can we build an decision graph for selection filtering that will be future proof?
I have build diagram to showcase how this would look like for current model

wtrocki · 2022-07-13T15:17:49Z

@rkpattnaik780 We are missing verification steps for @jackdelahunt

wtrocki · 2022-07-13T15:18:14Z

pkg/cmd/kafka/create/completions.go

-}
-
-// GetMarketplaceAcctIdCompletionValues returns a list of valid marketplace account IDs for the organization
-func GetMarketplaceAcctIdCompletionValues(f *factory.Factory) (validMarketplaceAcctIDs []string, directive cobra.ShellCompDirective) {


Reason to remove this?

wtrocki · 2022-07-13T20:54:28Z

pkg/cmd/kafka/create/create.go

+		validMarketPlaces := FetchValidMarketplaces(orgQuota.MarketplaceQuotas)
+		if len(validMarketPlaces) == 1 {
+			answers.Marketplace = validMarketPlaces[0]
+		} else {


How this would work with len(validMarketPlaces) == 0?

Ignore me. That would not be possible

wtrocki · 2022-07-13T20:58:48Z

pkg/shared/accountmgmtutil/ams.go

+			if marketplace.Provider != "" && marketplace.CloudAccountID != "" {
+				if *cloudAccount.CloudProviderId == marketplace.Provider && *cloudAccount.CloudAccountId == marketplace.CloudAccountID {
+					matchedQuotas = append(matchedQuotas, quota)
+				}
+			} else if marketplace.Provider != "" || marketplace.CloudAccountID != "" {


Arent those the same if checks?

This was to handle the case of providing only provider or account ID.

wtrocki · 2022-07-13T21:42:02Z

pkg/shared/accountmgmtutil/ams.go

 }

-func GetValidMarketplaces(ctx context.Context, connectionFunc factory.ConnectionFunc) (marketplaces []string, err error) {
+func pickCloudAccount(f *factory.Factory, cloudAccounts *[]amsclient.CloudAccount, market MarketplaceInfo) (*[]amsclient.CloudAccount, error) {


Should this method return single object instead of array. It does pick cloud account right?

Used array to make it easy to use with quotapsec object.

wtrocki · 2022-07-13T21:43:00Z

pkg/shared/accountmgmtutil/ams.go

+	var matchedAccounts []amsclient.CloudAccount
+
+	for _, cloudAccount := range *cloudAccounts {
+		if market.Provider != "" || market.CloudAccountID != "" {


Those variables are set outside for so we might do them once outside loop?

wtrocki · 2022-07-13T21:45:18Z

pkg/shared/accountmgmtutil/ams.go

+
+	for _, cloudAccount := range *cloudAccounts {
+		if market.Provider != "" || market.CloudAccountID != "" {
+			if *cloudAccount.CloudProviderId == market.Provider || *cloudAccount.CloudAccountId == market.CloudAccountID {


Question I have:
Do you think we should support people specifying only market.Provider?

Maybe we can just:

Suggested change

if *cloudAccount.CloudProviderId == market.Provider || *cloudAccount.CloudAccountId == market.CloudAccountID {

if *cloudAccount.CloudProviderId == market.Provider && *cloudAccount.CloudAccountId == market.CloudAccountID {

To keep this convoluted logic little bit simpler?

Do you think we should support people specifying only market.Provider?

I will agree not allowing marketplace provider alone can make the logic bit simpler. I will be removing it.

wtrocki · 2022-07-14T12:32:22Z

@rkpattnaik780 let's do end to end verification for this together.

jackdelahunt · 2022-07-14T12:46:11Z

Ran into an issue.

When only trial instances were available setting the billing model (standard or marketplace) did not throw an error as described in the verification steps.

rkpattnaik780 · 2022-07-14T12:52:00Z

Ran into an issue.

When only trial instances were available setting the billing model (standard or marketplace) did not throw an error as described in the verification steps.

Hi @jackdelahunt
Not able to reproduce it. Can you paste the command in verbose mode.

jackdelahunt · 2022-07-14T12:56:24Z

Here -> https://gist.github.com/jackdelahunt/81e45bfcbee10b38f9bdaab3e35800ca

rkpattnaik780 · 2022-07-14T13:17:07Z

Here -> https://gist.github.com/jackdelahunt/81e45bfcbee10b38f9bdaab3e35800ca

Thanks for the catch, PR has been updated.

jackdelahunt · 2022-07-14T13:46:12Z

From what I can do (step 2) LGTM 👍

wtrocki · 2022-07-14T14:23:54Z

I have tried to do "Duncan Test" as start:

[wtrocki@graphapi app-services-cli (pr/rkpattnaik780/1636 ✗)]$ rhoas kafka create                   
? Name: duncan-test
? Cloud Provider: aws
? Billing Model: marketplace
? Cloud Region: us-east-1
Kafka instance with size 1 is being created. (Size id: x1)
❌ 400 Bad Request. Run the command in verbose mode using the -v flag to see more information

It has:

Service API user (Enables marketplace, but no standard) - standard was still present
Wrong plan selected (developer plan/trial was on where I selected proper billing model (marketplace)
Sizes provided were wrong only x1 (my plan had support up to x2)

Error detail:

{"reason":"Unable to detect instance type in plan provided: 'developer.x1'","operation_id":"cb82cemo4k31f4mqqo9g","id":"21","kind":"Error","href":"/api/kafkas_mgmt/v1/errors/21","code":"KAFKAS-MGMT-21"}

wtrocki · 2022-07-15T13:07:42Z

@rkpattnaik780 let's do more comprehensive testing with control plane team and mock. Once we have confidence in this we can release.

wtrocki · 2022-07-15T13:08:58Z

pkg/cmd/kafka/create/create.go

-		f.Logger.Debug(fmt.Sprintf("Selected quota object: %#v", userQuota))
+		userQuotaJSON, _ := json.MarshalIndent(userQuota, "", "  ")
+		f.Logger.Debug("Selected Quota object:")
+		f.Logger.Debug(string(userQuotaJSON))


Let's create helper for logging that type of stuff. It takes a lot of space.

feat(kafka create): add billing model logic

f1f5915

rkpattnaik780 requested a review from wtrocki July 12, 2022 10:00

openshift-ci bot added the do-not-merge/work-in-progress label Jul 12, 2022

wtrocki reviewed Jul 12, 2022

View reviewed changes

rkpattnaik780 added 3 commits July 12, 2022 17:30

fix: add condition for trial instance

c3d5ff0

feat(kafka create): handle interactive mode

783b891

refactor: add localizations to helper methods

fa8dc0c

rkpattnaik780 force-pushed the ams_billing_model branch from 8d44476 to fa8dc0c Compare July 13, 2022 12:28

fix: use answers struct for interactive

b2b5c63

rkpattnaik780 requested a review from wtrocki July 13, 2022 13:27

wtrocki requested a review from jackdelahunt July 13, 2022 15:17

wtrocki reviewed Jul 13, 2022

View reviewed changes

rkpattnaik780 added 3 commits July 14, 2022 13:25

feat(kafka create): restore flag completions

440c724

fix: markeplace and account id should be supplied together

c95561c

fix: localize error message

cf00270

rkpattnaik780 added 2 commits July 14, 2022 15:47

fix: interactive mode for trial instances

3c3c4e1

fix: add dynamic completion for billing model flag

0902b4a

rkpattnaik780 requested a review from wtrocki July 14, 2022 11:15

rkpattnaik780 changed the title ~~wip: feat(kafka create): add billing model logic~~ feat(kafka create): add billing model logic Jul 14, 2022

rkpattnaik780 marked this pull request as ready for review July 14, 2022 11:15

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 14, 2022

fix: diable interactive if additional flag is provided

c8a633e

wtrocki and others added 2 commits July 15, 2022 10:32

fix: minor debug statements

79a97f2

fix: pass billing details to payload in interactive mode

47368d2

rkpattnaik780 merged commit fb51888 into redhat-developer:main Jul 15, 2022

wtrocki reviewed Jul 15, 2022

View reviewed changes


		}

		if userInstanceType.BillingModel == "standard" \|\| userInstanceType.BillingModel == "marketplace" {

	if cloudAccount.CloudProviderId == market.Provider \|\| cloudAccount.CloudAccountId == market.CloudAccountID {
	if cloudAccount.CloudProviderId == market.Provider && cloudAccount.CloudAccountId == market.CloudAccountID {

Conversation

rkpattnaik780 commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verification Steps

Type of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki commented Jul 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki commented Jul 14, 2022

Uh oh!

jackdelahunt commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkpattnaik780 commented Jul 14, 2022

Uh oh!

jackdelahunt commented Jul 14, 2022

Uh oh!

rkpattnaik780 commented Jul 14, 2022

Uh oh!

jackdelahunt commented Jul 14, 2022

Uh oh!

wtrocki commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wtrocki commented Jul 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rkpattnaik780 commented Jul 12, 2022 •

edited

Loading

wtrocki Jul 12, 2022 •

edited

Loading

jackdelahunt commented Jul 14, 2022 •

edited

Loading

wtrocki commented Jul 14, 2022 •

edited

Loading