refactor(google): resources migration batch 9

[sinabakh]

No description provided.

[sinabakh]

@aliscott Would you please review?

[aliscott]

LGTM

[aliscott]

Should we add region, name, etc to the struct and rename this function to BuildResource so it's like the other resources?

[aliscott]

Although this isn't related to these changes, it might be good to document what gRegion and fixedRegion are. It's not immediately clear to me looking through the code.

[aliscott]

Suggested change

	var usage float64
	var used float64
	var lastEndUsageAmount float64
	usage = GetFloatFieldValueByUsageTag(usageKey, *r)
	var used float64
	var lastEndUsageAmount float64
	usage := GetFloatFieldValueByUsageTag(usageKey, *r)

[aliscott]

Now that we have the NetworkEgressUsage struct I'm wondering if it enables different approaches for handling the different egress resource types.

One option could be to use composition to extend the NetworkEgressUsage type, e.g:

type ComputeVPNGatewayNetworkEgressUsage struct {
  NetworkEgressUsage
}

func (r *ComputeVPNGatewayNetworkEgressUsage) getEgressRegionsData...

Another option could be to have an interface for these and pass it to this struct instead of egressResourceType, e.g:

type EgressResource interface {
  includesSameContinent() bool
  getEgressRegionsData []*EgressRegionData
}

What do you think?

[sinabakh]

@aliscott Ok so your comments started a rabbit hole of refactoring :))
I want to give a summary of the changes and their reason.

A. Why the previous structure was wrong

In the previous refactoring, we had a single struct named NetworkEgress and all usage data were its attributes. The problem was that different resources have different usage keys. For instance, VPNGateway has no same_continent usage key. So we should probably have different structs for different types of resources that use egress.

B. Why composition didn't work

Each egress resource has about 5-6 usage keys. To make working with them easier I use reflect to get their values so I can iterate over them. The helper function is called GetFloatFieldValueByUsageTag.
Let's say we have a core struct named NetworkEgress and another one that encapsulates it named ComputeVPNGatewayNetworkEgressUsage. Since the core logic is placed on the NetworkEgress it can't access its parent fields unless the fields are defined on the core struct which then the problem A will arise.

So I had to split most of the code and introduce some duplication. I'm not sure if that's the best or even the right way so I would be more than happy to get your ideas.

[aliscott]

This looks good. I think separating it out makes it more explicit even if it is duplicated. One thought I had was if we should inline the usage subresources in each resource. I've written out an example here since it was easier to express as code: https://gist.github.com/aliscott/6d7f8973f29f2d4a5b1dabda3bd4dc7d. I feel that putting it inline means less jumping around when looking at the code.

Other suggestions I have:

1. Remove the getEgressAPIRegionName and getEgressAPIServiceName functions if we're just inlining these
2. Same with the Prefix and Address properties of the usage struct. And if we remove them we don't need the NetworkEgressUsage struct anymore, we can just inline Region where we need it.

Let me know what you think.

[aliscott]

Suggested change

	func stepPricingHelper(usage float64, usageFiltersData []*egressRegionUsageFilterData, regData *egressRegionData, defaultAPIRegionName, serviceName *string) []*schema.CostComponent {
	func egressStepPricingHelper(usage float64, usageFiltersData []*egressRegionUsageFilterData, regData *egressRegionData, defaultAPIRegionName, serviceName string) []*schema.CostComponent {

1. I'd make the name more explicit since the function is global in this package
2. I'd suggest making making the region and service strings, since we're just wrapping them in a ptr to pass them in here which seems pointless.

[sinabakh]

Makes total sense

[aliscott]

Can field.Elem() be nil? We might want to check that just in case.

[sinabakh]

Checks on previous parts makes it logically impossible for it to panic.

[aliscott]

Do we need a check for this being nil, e.g.:

if r.MonthlyEgressDataTransferGB == nil {
        r.MonthlyEgressDataTransferGB = &computeExternalVPNGatewayNetworkEgressUsage{}
}

[sinabakh]

I think the call to PopulateArgsWithUsage will take care of initializing it.

[sinabakh]

This looks good. I think separating it out makes it more explicit even if it is duplicated. One thought I had was if we should inline the usage subresources in each resource. I've written out an example here since it was easier to express as code: https://gist.github.com/aliscott/6d7f8973f29f2d4a5b1dabda3bd4dc7d. I feel that putting it inline means less jumping around when looking at the code.

Other suggestions I have:

1. Remove the `getEgressAPIRegionName` and `getEgressAPIServiceName` functions if we're just inlining these

2. Same with the `Prefix` and `Address` properties of the usage struct. And if we remove them we don't need the `NetworkEgressUsage` struct anymore, we can just inline `Region` where we need it.

Let me know what you think.

I'm a big fan of making things simple so I completely agree with you.

[vdmgolub]

@sinabakh Just FYI in case you plan on the next batch, I'm working on migrating container_cluster/container_node_pool Google resources and some other related to them.

[aliscott]

LGTM thanks Sina!