OCM 3768 | Feat | Filtered OCM versions for marketplace gcp clusters #556
Conversation
pkg/cluster/versions.go
Outdated
| filter := "enabled = 'true'" | ||
| filter := fmt.Sprintf("enabled = 'true' AND gcp_marketplace_enabled = '%s'", gcpMarketplaceEnabled) | ||
| if channelGroup != "" { | ||
| filter = fmt.Sprintf("%s AND channel_group = '%s'", filter, channelGroup) | ||
| } |
There was a problem hiding this comment.
it would be probably safer to only set the gcp_marketplace_enabled restriction if a value is explicitly provided (like channel group).
I would keep filter := "enabled = 'true'" as the base query only.
cmd/ocm/list/version/cmd.go
Outdated
|
|
||
| fs.StringVar( | ||
| &args.gcpMarketplaceEnabled, | ||
| "gcp-marketplace-enabled", |
There was a problem hiding this comment.
this is a long name for the flag. Maybe gcp-marketplace or marketplace-gcp (to match the subscription type value) would be enough?
There was a problem hiding this comment.
As per the suggestion (https://redhat-internal.slack.com/archives/C05BJQLPYQ5/p1695828401018519?thread_ts=1695762022.253959&cid=C05BJQLPYQ5), changed the flag to gcp-marketplace
pkg/cluster/versions.go
Outdated
| versions []string, defaultVersion string, err error) { | ||
| collection := client.Versions() | ||
| page := 1 | ||
| size := 100 | ||
| filter := "enabled = 'true'" | ||
| filter = fmt.Sprintf("%s AND gcp_marketplace_enabled = '%s'", filter, gcpMarketplaceEnabled) |
There was a problem hiding this comment.
I don't think we should filter for gcp_marketplace_enabled as long the flag is explicitly set.
You can probably achieve this by using a pointer and checking for nil.
cmd/ocm/create/cluster/cmd.go
Outdated
| @@ -400,16 +401,16 @@ func GetDefaultClusterFlavors(connection *sdk.Connection, flavour string) (dMach | |||
| } | |||
|
|
|||
| func getVersionOptions(connection *sdk.Connection) ([]arguments.Option, error) { | |||
| options, _, err := getVersionOptionsWithDefault(connection, "") | |||
| options, _, err := getVersionOptionsWithDefault(connection, "", "false") | |||
There was a problem hiding this comment.
I don't think we should set the default value to this flag
cmd/ocm/list/version/cmd.go
Outdated
| fs.StringVar( | ||
| &args.gcpMarketplace, | ||
| "gcp-marketplace", | ||
| "false", |
There was a problem hiding this comment.
is this the default value?
There was a problem hiding this comment.
Yes, changed to an empty string
cmd/ocm/create/cluster/cmd.go
Outdated
| gcpMarketplaceEnabled := strconv.FormatBool(isGcpMarketplaceSubscriptionType) | ||
| versions, defaultVersion, err := getVersionOptionsWithDefault(connection, args.channelGroup, | ||
| &gcpMarketplaceEnabled) |
There was a problem hiding this comment.
This will cause filtering by gcp_marketplace_enabled = 'false' when selecting billing models other than marketplace-gcp - which is not the goal, PTAL
cmd/ocm/create/cluster/cmd.go
Outdated
| @@ -400,16 +401,16 @@ func GetDefaultClusterFlavors(connection *sdk.Connection, flavour string) (dMach | |||
| } | |||
|
|
|||
| func getVersionOptions(connection *sdk.Connection) ([]arguments.Option, error) { | |||
| options, _, err := getVersionOptionsWithDefault(connection, "") | |||
| options, _, err := getVersionOptionsWithDefault(connection, "", nil) | |||
There was a problem hiding this comment.
since the default value of the flag is an empty string, does it make sense to use the same pattern here and change the type from *string to string?
There was a problem hiding this comment.
Yes, changed to keep a string instead of *string
cmd/ocm/list/version/cmd.go
Outdated
|
|
||
| fs.StringVar( | ||
| &args.gcpMarketplace, | ||
| "gcp-marketplace", |
There was a problem hiding this comment.
should this be marketplace-gcp instead?
|
LGTM |
Changed
Tested