feat: add flag that checks whether any of the plans have been deactivated #115

zucchinidev · 2023-12-07T17:29:54Z

Checklist:

Have you added Release Notes in the docs repositories?
Have you followed the Conventional Commits specification?

…ated A new flag has been added. The flag `check-deactivated-plans` checks whether any of the plans have been deactivated. If any deactivated plans are found, the execution will fail. [#186597655](https://www.pivotaltracker.com/story/show/186597655)

cf-gitbot · 2023-12-07T17:29:57Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186638806

The labels on this github issue will be updated when the story is started.

[#186597655](https://www.pivotaltracker.com/story/show/186597655)

blgm

Mostly this is great. There are a couple of things that definitely need to be resolved, most of my comments are stylistic and can be ignored.

README.md

blgm · 2023-12-07T18:44:53Z

internal/ccapi/service_instances_test.go

@@ -420,3 +447,173 @@ func fakeResponse() string {
 }
 `
 }
+
+// [FAILED] Expected


I think this was probably not meant to be in the commit.

blgm · 2023-12-07T22:52:46Z

internal/ccapi/service_instances.go

-		receiver.Instances[i].OrganizationGUID = orgGUID
-		receiver.Instances[i].OrganizationName = orgName
+	for _, instance := range receiver.Instances {
+		plan := getPlanByGUID(plans, instance.ServicePlanGUID)


The loop in this function will be run for every service instance, so up to 5000 times. If the number of plans is small, it might be more efficient than the map lookup that it replaced. I think this is fine, it's just good to be aware of situations where there's a loop within a loop.

You're totally right. I'm going to improve this part to avoid the loop in each iteration

Reviewing the code after your suggestion, I realised that we can simplify the query parameter and the receiver.

blgm · 2023-12-07T22:53:37Z

internal/ccapi/service_instances.go

+		instance.SpaceName = spaceName
+		instance.OrganizationGUID = orgGUID
+		instance.OrganizationName = orgName
+		instances = append(instances, instance)


Having two slices will allocate twice as much memory. Again because we are currently limited to 5000 instances (as pagination is not implemented), I think this will not be a problem.

Thanks for this advice. I usually prefer using value semantics instead of pointer semantics, but I think you are right. I know we’re not going to overflow memory by being limited to 5000 instances at most, but I’m going to change it. In this way we will avoid future problems if someone changes the number 5000 to something greater. On the contrary, if we implement pagination, it will be a perfect time to use value semantics

blgm · 2023-12-07T22:54:32Z

internal/ccapi/service_instances.go

+		instance.ServicePlanName = plan.Name
+		instance.ServiceOfferingGUID = plan.ServiceOffering.GUID
+		instance.ServiceOfferingName = plan.ServiceOffering.Name
+		instance.ServicePlanMaintenanceInfoVersion = plan.MaintenanceInfoVersion


Doing the decoration with service plan data here is much nicer than the old implementation.

blgm · 2023-12-07T23:08:55Z

internal/upgrader/upgrader.go

 	}
 }

-func performUpgrade(api CFClient, upgradableInstances []ccapi.ServiceInstance, planVersions map[string]string, parallelUpgrades int, log Logger) error {
+func checkDeactivatedPlans(log Logger, upgradableInstances []ccapi.ServiceInstance) error {
+	var ii []struct{}


I'm curious as to why this method was chosen rather than having a flag or counter?

deactivatedPlanFound := false

or

deactivatedPlansFound := 0

I initially added the deactivated plans and logic to add the deactivated offerings. I deleted the offerings code and I did not change the code. As you suggested, I see more suitable to use the boolean flag deactivatedPlanFound := false

Thank you very much!

blgm · 2023-12-07T23:09:27Z

internal/upgrader/upgrader.go

@@ -74,7 +103,7 @@ func performUpgrade(api CFClient, upgradableInstances []ccapi.ServiceInstance, p
 				UpgradeableIndex:       i,
 				ServiceInstanceName:    instance.Name,
 				ServiceInstanceGUID:    instance.GUID,
-				MaintenanceInfoVersion: planVersions[instance.ServicePlanGUID],


Much better!

blgm · 2023-12-07T23:10:33Z

internal/upgrader/upgrader.go

@@ -118,7 +147,7 @@ func performDryRun(upgradableInstances []ccapi.ServiceInstance, log Logger) erro
 	return nil
 }

-func discoverServicePlans(api CFClient, brokerName string) (map[string]string, error) {
+func discoverServicePlans(api CFClient, brokerName string) ([]ccapi.ServicePlan, error) {


I wonder whether it could make sense to inline this function?

Let me refactor it and see the benefits. I thought about it because now, the logic in the function is simple, but I did not want to change more code than needed.

Much better. I added the refactor

blgm · 2023-12-07T23:11:53Z

internal/upgrader/upgrader_test.go

+						Available:              false,
+						Name:                   "fake-deactivated-plan-1-name",
+						MaintenanceInfoVersion: "1.5.7",
+						ServiceOffering: ccapi.ServiceOffering{


Here's an example of why it could be cleaner to flatten the ServiceOffering object into just two strings.

blgm · 2023-12-07T23:12:36Z

internal/upgrader/upgrader_test.go

+		})
+
+		When("there are no deactivated plans", func() {
+			It("does the next thing", func() {


Need to write this test and give it a better name. The aim is to check that if there are no deactivated plans, then it does not fail, and instead performs the next action - such as starting an upgrade or doing a dry run. Could potentially also be tested by running all the other tests with --check-deactivated-plans

Oh yes. Spot on.

Co-authored-by: George Blue <blgm@users.noreply.github.com>

The loop in the function was executed for every service instance, so up to 5000 times. Probably, the number of plans is small, and we believe it is more efficient a map lookupd. Because we populate the instance with the plan, we no longer needed some fields. We simplify the query params and reduce network band usage. [#186597655](https://www.pivotaltracker.com/story/show/186597655)

We use pointer semantics instead of value semantics. We may change it in the future if we implement pagination. [#186597655](https://www.pivotaltracker.com/story/show/186597655)

[#186597655](https://www.pivotaltracker.com/story/show/186597655)

zucchinidev · 2023-12-08T16:36:02Z

Thank you very much @blgm for your help

blgm and others added 2 commits December 4, 2023 15:46

WIP

e4f202b

cf-gitbot added the unscheduled label Dec 7, 2023

fix: typo

081afa1

[#186597655](https://www.pivotaltracker.com/story/show/186597655)

blgm requested changes Dec 7, 2023

View reviewed changes

zucchinidev and others added 8 commits December 8, 2023 12:04

chore: reword flag description

dfc9a58

Co-authored-by: George Blue <blgm@users.noreply.github.com>

chore: two slices allocate much more memory

06d4a11

We use pointer semantics instead of value semantics. We may change it in the future if we implement pagination. [#186597655](https://www.pivotaltracker.com/story/show/186597655)

chore: flat service offering struct to simplify code

ba7dc40

[#186597655](https://www.pivotaltracker.com/story/show/186597655)

fix: set correct service offering guid

7703e6b

[#186597655](https://www.pivotaltracker.com/story/show/186597655)

test: format JSON

68e144e

[#186597655](https://www.pivotaltracker.com/story/show/186597655)

test: remove unnecessary comment

c5aa120

[#186597655](https://www.pivotaltracker.com/story/show/186597655)

test: happy path test

73af4e8

[#186597655](https://www.pivotaltracker.com/story/show/186597655)

zucchinidev requested a review from blgm December 8, 2023 16:36

blgm approved these changes Dec 11, 2023

View reviewed changes

blgm merged commit ce54f11 into main Dec 11, 2023
6 checks passed

cf-gitbot removed the unscheduled label Dec 11, 2023

blgm deleted the deactivated-plans branch December 11, 2023 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add flag that checks whether any of the plans have been deactivated #115

feat: add flag that checks whether any of the plans have been deactivated #115

zucchinidev commented Dec 7, 2023 •

edited

Loading

cf-gitbot commented Dec 7, 2023

blgm left a comment

blgm Dec 7, 2023

blgm Dec 7, 2023

zucchinidev Dec 8, 2023

zucchinidev Dec 8, 2023

zucchinidev Dec 8, 2023

blgm Dec 7, 2023

zucchinidev Dec 8, 2023

blgm Dec 7, 2023

blgm Dec 7, 2023

zucchinidev Dec 8, 2023

zucchinidev Dec 8, 2023

blgm Dec 7, 2023

blgm Dec 7, 2023

zucchinidev Dec 8, 2023 •

edited

Loading

zucchinidev Dec 8, 2023

blgm Dec 7, 2023

blgm Dec 7, 2023

zucchinidev Dec 8, 2023

zucchinidev commented Dec 8, 2023

feat: add flag that checks whether any of the plans have been deactivated #115

feat: add flag that checks whether any of the plans have been deactivated #115

Conversation

zucchinidev commented Dec 7, 2023 • edited Loading

Checklist:

cf-gitbot commented Dec 7, 2023

blgm 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

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

zucchinidev Dec 8, 2023 • edited Loading

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

zucchinidev commented Dec 8, 2023

zucchinidev commented Dec 7, 2023 •

edited

Loading

zucchinidev Dec 8, 2023 •

edited

Loading