fix(kafka create): reduce number of calls made to AMS API #1596

rkpattnaik780 · 2022-06-13T07:59:25Z

Re-use QuotaCostList wherever possible.

Follow up for #1593

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
Enhancement

wtrocki · 2022-06-14T09:32:43Z

pkg/cmd/kafka/create/completions.go

+	quotaCostList, err := accountmgmtutil.FetchOrgQuotaCost(f.Context, conn)
+	if err != nil {
+		return nil, directive
+	}
+
+	userInstanceType, _ := accountmgmtutil.GetUserSupportedInstanceType(quotaCostList, &constants.Kafka.Ams)


There are two ways to look at this:

We can make single call and return structure that contains all high level objects
our create command needs

Have one call that returns some raw api object that contains a lot of non useful data

Here we are using approach nr2. While it is ok this has some problems:

2 calls needed - result of the first call exposes too many fields that are low level.
There is no enough encapsulation

Verbose usage - 2 calls needed - error handling is not well defined and will return
low level info to user

Not clear what method to use to get high level info for the CLI

While this PR is acceptable I personally feel that we should be using encapsulation more.
This means that we should avoid using:

GetValidMarketplaces(quotaCostList *amsclient.QuotaCostList)

And look to create our own CLi structures that represent values that we really want to operate in CLI

There was already existing structure that we were returning from the AMS calls that contained type of quota and remaining amount. I think we should extend that structure:

https://github.com/redhat-developer/app-services-cli/blob/main/pkg/shared/accountmgmtutil/ams.go#L38

jackdelahunt · 2022-06-15T10:11:41Z

@rkpattnaik780 I was requested to review this but are you still making changes that Wojciech suggested?

wtrocki · 2022-06-15T10:14:55Z

Let's ignore reviewing this for now

pkg/cmd/kafka/create/create.go

pkg/shared/accountmgmtutil/ams.go

rkpattnaik780 · 2022-06-27T13:46:10Z

pkg/cmd/kafka/create/create.go

@@ -225,7 +234,7 @@ func runCreate(opts *options) error {
 			return f.Localizer.MustLocalizeError("kafka.create.error.noInteractiveMode")
 		}

-		payload, err = promptKafkaPayload(opts, *userInstanceType)
+		payload, err = promptKafkaPayload(opts, userInstanceType)


The interactive mode needs more changes, currently the user will need to specify BillingModel as flag before starting the interactive mode.

pkg/cmd/kafka/create/api_validators.go

wtrocki · 2022-06-27T17:10:33Z

pkg/cmd/kafka/create/create.go

 		if err != nil || userInstanceType == nil {
 			return f.Localizer.MustLocalizeError("kafka.create.error.userInstanceType.notFound")
 		}
+
+		if opts.billingModel == "marketplace" {


I would see this logic happening outside create command

pkg/cmd/kafka/create/create.go

wtrocki · 2022-06-27T17:13:39Z

pkg/cmd/kafka/create/create.go

+
+	billingModelPrompt := &survey.Select{
+		Message: "Billing model:",
+		Options: kafkacmdutil.ValidBillingModels,


That will outdate fast as new billing models should be introduced.
I think the best tradeoffs would be to return error to user from ams package containing billing models he can create when invalid is provided or billing model is missing.

pkg/cmd/kafka/create/data.go

pkg/cmd/kafka/kafkacmdutil/kafka_util.go

wtrocki · 2022-06-27T17:16:59Z

pkg/shared/accountmgmtutil/ams.go

 }

-func GetUserSupportedInstanceType(ctx context.Context, spec *remote.AmsConfig, conn connection.Connection) (quota *QuotaSpec, err error) {
-	userInstanceTypes, err := GetUserSupportedInstanceTypes(ctx, spec, conn)
+func GetUserSupportedInstanceType(ctx context.Context, spec *remote.AmsConfig, conn connection.Connection, billingModel string) (quota *QuotaSpec, err error) {


This method should probably include marketplaceId and marketplaceType. Just billing model would not be enough for us to return single quota (there will be situations when multiple quotas are returned

pkg/shared/accountmgmtutil/ams.go

wtrocki · 2022-06-27T17:18:23Z

pkg/shared/accountmgmtutil/ams.go

-func BattleOfInstanceBillingModels(quotas []QuotaSpec) []QuotaSpec {
-	var betterQuotasMap map[string]*QuotaSpec = make(map[string]*QuotaSpec)
-	alwaysWinsBillingModel := "standard"
+func BattleOfInstanceBillingModels(quotas []QuotaSpec, alwaysWinsBillingModel string) []QuotaSpec {


This method should be removed.

pkg/shared/accountmgmtutil/api.go

wtrocki · 2022-06-27T17:22:58Z

I think PR trying to do too much. I would suggest to:

Not change too much code ouside accountmgmt package.
Remove logic for picking instance types etc.
Have new method for determining quota type:

func getMeQuotaFor(billingType *string, 
       marketplaceProvider *string, 
      cloudAccountId *string) (*Quota, err) {
     // match all arguments to the 
     // quota objects and return single one
     // return error if: cannot find quota object 
     // and size of quota object is larger than 1.
}

create.go changes should only contain adding 3 new flags and pass them to AMS. None of the existing logic should change.
Additionally create.go should also pass all those values to backend.
I think we are fine (for now) to keep billing model outside interactive mode/do not support bash suggestions for 3 of those flags ( I see you commented them)

wtrocki · 2022-06-27T17:23:14Z

@rkpattnaik780 hope that helps

pkg/shared/accountmgmtutil/ams.go

wtrocki · 2022-06-28T16:29:37Z

pkg/shared/accountmgmtutil/ams.go

+		return nil, errors.New("accountID cant be provided with standard billing model")
+	}
+
+	if (cloudAccountID != "") != (marketplace != "") {


Very clever pattern!

wtrocki

I love how simple and elegant this PR is
How it handles all business cases - Now and Future!
How clean and future proof this code looks like
Reusability of this code across services

Reviewed!
Verified!
Amazed!

wtrocki · 2022-06-28T16:49:05Z

I have noticed that this does not work with Trial instances. I can fix that on friday without worries

)" This reverts commit 5346fc1.

)" (#1616) This reverts commit 5346fc1.

… API (#1596)" (#1616)" This reverts commit 955dd6f.

… API (#1596)" (#1616)" (#1619) This reverts commit 955dd6f.

)" (#1616) This reverts commit 5346fc1.

wtrocki requested review from wtrocki and jackdelahunt June 14, 2022 09:27

wtrocki reviewed Jun 14, 2022

View reviewed changes

feat(kafka create): add billing model flag

136e35d

rkpattnaik780 force-pushed the reduce_ams_calls branch from 3c4b032 to 136e35d Compare June 23, 2022 10:31

jackdelahunt reviewed Jun 23, 2022

View reviewed changes

pkg/cmd/kafka/create/create.go Outdated Show resolved Hide resolved

feat: add logic to pick the quotacost object

4daddcb

rkpattnaik780 commented Jun 27, 2022

View reviewed changes

pkg/shared/accountmgmtutil/ams.go Show resolved Hide resolved

rkpattnaik780 commented Jun 27, 2022

View reviewed changes

wtrocki reviewed Jun 27, 2022

View reviewed changes

pkg/cmd/kafka/create/api_validators.go Show resolved Hide resolved

wtrocki reviewed Jun 27, 2022

View reviewed changes

pkg/cmd/kafka/create/create.go Outdated Show resolved Hide resolved

wtrocki reviewed Jun 27, 2022

View reviewed changes

pkg/cmd/kafka/create/data.go Outdated Show resolved Hide resolved

wtrocki reviewed Jun 27, 2022

View reviewed changes

pkg/cmd/kafka/kafkacmdutil/kafka_util.go Show resolved Hide resolved

wtrocki reviewed Jun 27, 2022

View reviewed changes

pkg/shared/accountmgmtutil/ams.go Outdated Show resolved Hide resolved

wtrocki reviewed Jun 27, 2022

View reviewed changes

pkg/shared/accountmgmtutil/api.go Outdated Show resolved Hide resolved

fix: change logic to pick up the quota

2aa6add

rkpattnaik780 commented Jun 28, 2022

View reviewed changes

pkg/shared/accountmgmtutil/ams.go Show resolved Hide resolved

rkpattnaik780 changed the title ~~fix(kafka create): reduce number of calls made to AMS API~~ WIP: fix(kafka create): reduce number of calls made to AMS API Jun 28, 2022

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

fix: handle multiple quota cost objects

9affd6d

wtrocki reviewed Jun 28, 2022

View reviewed changes

wtrocki approved these changes Jun 28, 2022

View reviewed changes

Merge branch 'main' into reduce_ams_calls

8263f52

wtrocki changed the title ~~WIP: fix(kafka create): reduce number of calls made to AMS API~~ fix(kafka create): reduce number of calls made to AMS API Jun 28, 2022

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

fix: mod sum

ec105c8

wtrocki merged commit 5346fc1 into main Jun 28, 2022

wtrocki deleted the reduce_ams_calls branch June 28, 2022 16:41

wtrocki added a commit that referenced this pull request Jun 28, 2022

Revert "fix(kafka create): reduce number of calls made to AMS API (#1596

bd507a6

)" This reverts commit 5346fc1.

wtrocki mentioned this pull request Jun 28, 2022

Revert "fix(kafka create): reduce number of calls made to AMS API" #1616

Merged

wtrocki added a commit that referenced this pull request Jun 28, 2022

Revert "fix(kafka create): reduce number of calls made to AMS API (#1596

955dd6f

)" (#1616) This reverts commit 5346fc1.

wtrocki added a commit that referenced this pull request Jun 29, 2022

Revert "Revert "fix(kafka create): reduce number of calls made to AMS…

cb9dec8

… API (#1596)" (#1616)" This reverts commit 955dd6f.

wtrocki added a commit that referenced this pull request Jun 29, 2022

Revert "Revert "fix(kafka create): reduce number of calls made to AMS…

ab05645

… API (#1596)" (#1616)" (#1619) This reverts commit 955dd6f.

wtrocki added a commit that referenced this pull request Jul 1, 2022

Revert "fix(kafka create): reduce number of calls made to AMS API (#1596

481c8c8

)" (#1616) This reverts commit 5346fc1.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(kafka create): reduce number of calls made to AMS API #1596

fix(kafka create): reduce number of calls made to AMS API #1596

rkpattnaik780 commented Jun 13, 2022

wtrocki Jun 14, 2022 •

edited

Loading

wtrocki Jun 14, 2022

jackdelahunt commented Jun 15, 2022

wtrocki commented Jun 15, 2022

rkpattnaik780 Jun 27, 2022

wtrocki Jun 27, 2022

wtrocki Jun 27, 2022

wtrocki Jun 27, 2022

wtrocki Jun 27, 2022

wtrocki commented Jun 27, 2022

wtrocki commented Jun 27, 2022

wtrocki Jun 28, 2022

wtrocki left a comment •

edited

Loading

wtrocki commented Jun 28, 2022

fix(kafka create): reduce number of calls made to AMS API #1596

fix(kafka create): reduce number of calls made to AMS API #1596

Conversation

rkpattnaik780 commented Jun 13, 2022

Type of change

wtrocki Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

wtrocki Jun 14, 2022

Choose a reason for hiding this comment

jackdelahunt commented Jun 15, 2022

wtrocki commented Jun 15, 2022

rkpattnaik780 Jun 27, 2022

Choose a reason for hiding this comment

wtrocki Jun 27, 2022

Choose a reason for hiding this comment

wtrocki Jun 27, 2022

Choose a reason for hiding this comment

wtrocki Jun 27, 2022

Choose a reason for hiding this comment

wtrocki Jun 27, 2022

Choose a reason for hiding this comment

wtrocki commented Jun 27, 2022

wtrocki commented Jun 27, 2022

wtrocki Jun 28, 2022

Choose a reason for hiding this comment

wtrocki left a comment • edited Loading

Choose a reason for hiding this comment

wtrocki commented Jun 28, 2022

wtrocki Jun 14, 2022 •

edited

Loading

wtrocki left a comment •

edited

Loading