Refactor Azure provider into pkg/cloud/azure

[babbageclunk]

What does this PR change?

It combines the Azure cloud provider and pkg/cloud/azurepricesheet into one more-coherent pkg/cloud/azure package. To avoid circular imports between pkg/cloud and pkg/cloud/azure the core types shared between the providers have been pulled out into pkg/cloud/models.

graph LR;
subgraph before
cloud --> azp[cloud/azurepricesheet];
end
subgraph oops[circular imports]
c2[cloud] --> az[cloud/azure]
az -- uh oh! --> c2
end

subgraph after
c3[cloud]-->models[cloud/models]
c3 --> az2[cloud/azure]
az2 --> models
end

before --> oops --> after

The type parameter on pkg/cloud/azurepricesheet.Downloader was a workaround to avoid an import cycle as well, so it's been removed from pkg/cloud/azure.PriceSheetDownloader.

Does this PR relate to any other PRs?

• Support for Azure consumption pricesheet download API #1842 - based on discussion from @ameijer during review.

How will this PR impact users?

• No behaviour change, this is purely code reorganisation.

Does this PR address any GitHub or Zendesk issues?

• No.

How was this PR tested?

• Running unit tests and running costmodel locally with rate card and price sheet downloading turned on, checking that both of those worked.

Does this PR require changes to documentation?

• No.

Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next Opencost release? If not, why not?

• I think it should have "next release", but it doesn't seem I can change labels.

[mattray]

@Sean-Holcomb @Pokom does this relate to #1787 and #1788?

[ameijer]

@mattray this is a follow up from our discussion here: #1842 (review)

[babbageclunk]

Oh darn, I hadn't seen that PR - I'm going to look through it now and see if I can harmonise this more.

[babbageclunk]

Ok - it looks like @Pokom and I took very similar approaches, so the PRs are pretty compatible. The differences as I see them:

1. Where this PR has pkg/cloud/types, 1787 has pkg/cloud/models - I like that name better, I'll update this to match.
2. 1787 moves the *ClusterMeta functions to pkg/cloud/utils where I just dumped them into pkg/cloud/types with some reservations. I'll change this, although I'm always a bit uncomfortable calling a package utils - would pkg/cloud/meta be a better name?
3. I didn't move all of the providers since I was primarily focused on fixing the azurepricesheet wart I'd created, but that would have been a natural next step for the other ones.
4. Handling ProviderConfig - I introduced a ProviderConfig interface in types and updated the Azure provider to take the interface rather than a concrete type, and left the implementation where it was in pkg /cloud, while 1787 moved it into pkg/cloud/provider. I slightly prefer my one here, but not for any super firm reason - it just seems cleaner to leave it in the cloud package.

What do people think?

[babbageclunk]

I'll change this, although I'm always a bit uncomfortable calling a package utils - would pkg/cloud/meta be a better name?

I ended up keeping the name utils since ToTitle is also in there.

[Pokom]

@babbageclunk sorry for the delay here, was on vacation last week! My apologies for not taking better notes and dropping them into #1787 after meeting with @Sean-Holcomb, but the short of it was the refactor was welcomed but likely too large and would require a lot of testing on the kubecost team to see through. In general, we agreed upon the overall structure that both of our PR's happened to land upon. I'd love to see this PR work its way through and be the foundation for our to refactor the remaining cloud packages.

I can take a deeper look at your PR over the week, however your summary is pretty spot on. Here's my attempt to respond to each of your points:

1. pkg/cloud/types or pkg/cloud/models works for me, don't have a hard preference
2. I strongly favor pkg/cloud/meta over utils, at the time I just couldn't think of a better name
3. Starting small here has a better chance of seeing the refactor land
4. Leaving the provider in pkg/cloud makes sense as well, I don't see a need to have it in a dedicated model module

Love the contribution and I think it addresses a common challenge folks have. Really cool to see us both land on similar implementations 😉

[Sean-Holcomb]

These Changes look good, lets get you guys unblocked