Add flags for destroy cluster powervs command #1613
Conversation
✅ Deploy Preview for hypershift-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
dharaneeshvrd
force-pushed
the
powervs-destroy-cluster-flags
branch
from
f91ff54 to
914c1b9
Compare
| cmd.Flags().StringVar(&opts.PowerVSPlatform.ResourceGroup, "resource-group", opts.PowerVSPlatform.ResourceGroup, "IBM Cloud Resource group") | ||
| cmd.Flags().StringVar(&opts.InfraID, "infra-id", opts.InfraID, "Cluster ID with which to tag IBM Cloud resources") | ||
| cmd.Flags().StringVar(&opts.PowerVSPlatform.BaseDomain, "base-domain", opts.PowerVSPlatform.BaseDomain, "Cluster's base domain") | ||
| cmd.Flags().StringVar(&opts.PowerVSPlatform.Region, "region", opts.PowerVSPlatform.Region, "IBM Cloud region. Default is us-south") |
There was a problem hiding this comment.
why do we need region and VPCRegion?
why do we have 3 different things region, VPCRegion and Zone which has the same default "us-south"?
There was a problem hiding this comment.
Those are 3 diff fields, only for us-south, all 3 fields sharing the same value.
For osaka, region - osa, zone - osa21 & vpc-region - jp-osa.
https://cluster-api-ibmcloud.sigs.k8s.io/reference/regions-zones-mapping.html
There was a problem hiding this comment.
I see, thanks.
You mention default is "us-south", but I can't see that default being set?
There was a problem hiding this comment.
My bad, added now, thanks!
dharaneeshvrd
force-pushed
the
powervs-destroy-cluster-flags
branch
from
914c1b9 to
c1348ff
Compare
cmd/cluster/powervs/destroy.go
Outdated
| @@ -63,13 +77,13 @@ func DestroyCluster(ctx context.Context, o *core.DestroyOptions) error { | |||
| inputErrors = append(inputErrors, fmt.Errorf("infrastructure ID is required")) | |||
| } | |||
| if o.PowerVSPlatform.Region == "" { | |||
There was a problem hiding this comment.
Upper case seems right to me here even though is an Errorf they are capital letter words regardless.
There was a problem hiding this comment.
To be in consistent with other Errorf messages, modified this, reverted the changes.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dharaneeshvrd, enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
dharaneeshvrd
force-pushed
the
powervs-destroy-cluster-flags
branch
from
c1348ff to
0dab198
Compare
|
@dharaneeshvrd: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
|
/lgtm |
What this PR does / why we need it:
This PR trying to address this bug https://issues.redhat.com/browse/MULTIARCH-2692 for not handling the flags for destroy cluster powervs command.
Which issue(s) this PR fixes (optional, use
fixes #<issue_number>(, fixes #<issue_number>, ...)format, where issue_number might be a GitHub issue, or a Jira story:Fixes #
Checklist