Prompt user to provide a cluster when creating a pipeline #570
Conversation
JoeColeman95
left a comment
JoeColeman95
left a comment
There was a problem hiding this comment.
Hey Lizette, great work on this!
I've found a couple of things that seem blocking to me (pagination/the forced cluster), happy to discuss if you disagree on any of these, though 🙂
pkg/cmd/pipeline/create.go
Outdated
| func getClusters(ctx context.Context, f *factory.Factory) map[string]string { | ||
| clusters, _, err := f.RestAPIClient.Clusters.List(ctx, f.Config.OrganizationSlug(), nil) | ||
| if err != nil { | ||
| return map[string]string{} | ||
| } | ||
|
|
||
| if len(clusters) < 1 { | ||
| return map[string]string{} | ||
| } | ||
|
|
||
| clusterMap := make(map[string]string, len(clusters)) | ||
| for _, cluster := range clusters { | ||
| clusterMap[cluster.Name] = cluster.ID | ||
| } | ||
| return clusterMap | ||
| } |
There was a problem hiding this comment.
Blocking: We should paginate here as this will only fetch the first page of results which defaults to 30. In this case, it'd be widely working, but some users may have more than 30 clusters, which would mean they'd only see some of the results.
We have pagination in a few places, but it can vary quite a lot depending on the call. The best example of this I can see in this code base is in the filterPipelines function, which should be 1:1 transferable here.
pkg/cmd/pipeline/create.go
Outdated
| func getClusters(ctx context.Context, f *factory.Factory) map[string]string { | ||
| clusters, _, err := f.RestAPIClient.Clusters.List(ctx, f.Config.OrganizationSlug(), nil) | ||
| if err != nil { | ||
| return map[string]string{} |
There was a problem hiding this comment.
Minor: API errors are silently swallowed here. Not blocking, but could be confusing if the list call fails for network/auth reasons
| Options: clusterNames, | ||
| } | ||
| var selectedClusterName string | ||
| err := survey.AskOne(prompt, &selectedClusterName, survey.WithValidator(survey.Required)) |
There was a problem hiding this comment.
Design choice/blocking: This seems like it could be breaking for some users. The API at the moment sets cluster_id to optional, in this case, we're forcing an option if there is a cluster available, but what if an org has a cluster with 0 agents but 1+ unclustered agent(s)? We'd be forcing them to select an unusable cluster, should we allow skipping even if there's a cluster in this case to allow these cases? I think respecting the API is the best bet.
There was a problem hiding this comment.
Good catch! Yeah, I agree we should allow skipping even if there is a cluster available.
Co-authored-by: Joe Coleman <107399878+JoeColeman95@users.noreply.github.com>
The create pipeline API now requires a cluster_id to create the pipeline. The
bk cliwill now prompt the user to select from the org's existing clusters, or skip when the org was created with only an unclustered area.