Add IBM Cloud Service constants

[cjschaef]

Add constants for the IBM Cloud Services, in relation to the ability to override these service endpoints in operators and the installer.

Related: https://issues.redhat.com/browse/SPLAT-1097

[elmiko]

in general i think this is a fine place for these constants that aren't necessarily used by the API and also are not in an ibm sdk package.

curious to hear what @JoelSpeed thinks

[JoelSpeed]

Can you provide an example of where these constants will be used?

[cjschaef]

Sure, here are a few of the references that are currently dependent on "local" constants (local to the component/repo):
openshift/cluster-ingress-operator#990 (comment)

https://github.com/openshift/machine-api-provider-ibmcloud/pull/28/files#diff-a2b3026502f45b5797810e086569bc8efd35d897b81f11f5dcad9de8ea7e1a95R79-R86

https://github.com/openshift/installer/pull/7632/files#diff-0b98c83784b19fcbc6c922a2837517b15df4331ec67a851d806db02af4b45d86R102-R104
https://github.com/openshift/installer/pull/7632/files#diff-85f228845db9e8a91e165bd3666fdb3926f93b07b1dbbb83da767f147d6c5c4bR143-R197

[cjschaef]

To be clear, they would replace these constants in those examples above

• https://github.com/openshift/cluster-ingress-operator/pull/990/files#diff-cfa88f3094d390a95d1d9b230dadae57aefa04e78041f7697f941ca110f7bca4R726-R730
• https://github.com/openshift/machine-api-provider-ibmcloud/pull/28/files#diff-5b483eb62896888537cf60265174b1b7e4d4b5d6469c814cfdf4b710d5f1ce43R34-R43
• https://github.com/openshift/installer/pull/7632/files#diff-1f230cd3a19364b3e4e1ce25d27ecf5411c8ccc591c9ba09410d873323e6122bR76-R95

[elmiko]

given @JoelSpeed 's comment, i think it's worthwhile to add a comment here explaining what these are used for/by. eg something

// These constants are used by MAO, MCO, CIO, Installer to identify blah blah blah

[cjschaef]

Sure, will do

[cjschaef]

Updated

[JoelSpeed]

Given we now have a concrete list of supported constants, why can we not make the API an enum?

[cjschaef]

I will see if I can refactor that, given I would expect to make conversions to/from string, should those helped functions be placed alongside, or somewhere also in library-go, as I'd expect duplicate code in each consuming component.
Or if golang has a nice method to already support conversions, given the primary use is in openshift/api/config/v1/IBMCloudServiceEndpoint resources? I'll start with the former, and see where we go from there.

[elmiko]

@cjschaef imo, if we move this to an enum in openshift/api then we should put the helper functions there as well.

edit:
for example, look at these feature set builder helpers https://github.com/openshift/api/blob/master/config/v1/types_feature.go

[JoelSpeed]

Enum in openshift API should be kube compliant, which means PascalCase. For the IBM SDK you need a different format. So I'd expect a conversion util and consts that are IBM SDK compliant to exist where this PR proposes them today.

The conversion util would take in one of the enum values from openshift/api and convert it to the IBM SDK equivalent. It doesn't even need to have any care for wider types since the API enum should use a typed string alias.

[elmiko]

So I'd expect a conversion util and consts that are IBM SDK compliant to exist where this PR proposes them today.

just to be clear, you want to see the consts and util funcs here, and then the enum in the api package?

[JoelSpeed]

There should be two sets of constants. In the API, the enum will be constructed based on type aliases for the different services

// +kubebuilder:validation:Enum=VIP;...;...
type IBMCloudServiceName string

const (
  IBMCloudServiceVIP IBMCloudServiceName = "VIP"
...
)

This is what I expect to see in API, here, there should be something similar but for the IBM SDK versions (lower cased), and then a transform

func IBMSDKCloudServiceName(in configv1.IBMCloudServiceName) (string, error) {
  switch in {
    case configv1.IBMCloudServiceVIP:
      return IBMSDKCloudServiceVIP, nil
    ...
    default:
      return "", fmt.Errorf("unknown IBM Cloud service name: %s", in)
  }
}

[cjschaef]

Okay, I'll see about getting a new openshift/api PR open, to define the service names, and see about updating (or closing) this PR to ad the transform/helper functions here in library-go

Updated
And to be clear, the lowercasing was not a mandatory need, more of a simplification. So, we could rely on the PascalCase versions and update all downstream references, which would rely on the Enum at that point anyway.

[cjschaef]

I opened the openshift/api PR for the enum
openshift/api#1657

I will refactor this PR, which would strictly be the helper functions, as I don't have hard requirements to use all lowercase names, currently, if we will be refactoring downstream PR's as well (MAPI, installer, etc.).

Done

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cjschaef
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@cjschaef: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/verify	0e6fcc0	link	true	/test verify
ci/prow/unit	0e6fcc0	link	true	/test unit
ci/prow/verify-deps	0e6fcc0	link	true	/test verify-deps

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[cjschaef]

I don't know that these helper functions are required any longer, now that the IBMCloudServiceEndpoint.Name type has changed from string.

I'm going to close this PR, as downsteam uses of the new IBMCloudServiceName should simply be able to to compare against the enum, and no string conversion or name refactoring should be necessary.....I hope.