Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCM-11842 : Improve gcp cluster creation with psc -UX #681

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

ckandag
Copy link
Collaborator

@ckandag ckandag commented Oct 21, 2024

https://issues.redhat.com/browse/OCM-11842

  1. Make the interactive workflow for ClusterPrivacy/ BYO- VPC similar to UI
  2. For WIF clusters, Ensure PSC is set when Cluster is Private

@ckandag ckandag changed the title Improve gcp cluster creation with psc -UX OCM-11842 : Improve gcp cluster creation with psc -UX Oct 21, 2024
cmd/ocm/create/cluster/cmd.go Outdated Show resolved Hide resolved
cmd/ocm/create/cluster/cmd.go Outdated Show resolved Hide resolved
cmd/ocm/create/cluster/cmd.go Show resolved Hide resolved
cmd/ocm/create/cluster/cmd.go Outdated Show resolved Hide resolved
Copy link
Contributor

@JakobGray JakobGray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a dry-run flow with this branch and got the following

go run ./cmd/ocm create cluster -i --dry-run=true 
? cluster name jjjj
? Subscription type: standard (Annual: Fixed capacity subscription from Red Hat)
? Cloud provider: gcp
? CCS: Yes
? Authentication method: Workload Identity Federation (WIF)
? WIF Configuration: jg1021 (2ehvi64du4ahq77a63vf3gj70bm63bpa)
? Multiple AZ: No
? Secure boot support for Shielded VMs: No
? Region: us-east1
? OpenShift version: 4.17.0
? Compute machine type: n2-standard-4
? Enable autoscaling: No
? Compute nodes: 2
? Private cluster (optional): Yes
? Install into an existing VPC (optional): No
? Machine CIDR: 10.0.0.0/16
? Service CIDR: 172.30.0.0/16
? Pod CIDR: 10.128.0.0/14
? Host prefix: 23
? Domain Prefix: 
dry run: Would be successful.

Based on point 2 I should not be able to select No for the prompt Install into an existing VPC (optional)

Also there are typos in the PSC prompt

? PrivatSericeConnect ServiceAttachment Subnet: [? for help] 

cmd/ocm/create/cluster/cmd.go Outdated Show resolved Hide resolved
Comment on lines 58 to 59
controlPlaneSubnetFlag = "compute-subnet"
computePlaneSubnetFlag = "control-plane-subnet"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two const names are mixed up for their values and not used in defining the flag.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are adding consts for these, we should replace the hard-coded values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just replaced in the code I was working on but yes I replace all

cmd/ocm/create/cluster/cmd.go Outdated Show resolved Hide resolved
Copy link
Contributor

@JakobGray JakobGray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment is still mostly the same

ocm create cluster -i --dry-run=true --ccs  --provider=gcp --region=us-east1 --compute-machine-type=n2-standard-4 --psc-subnet=jdg-21-tf2d8-worker-subnet jjjj
? Subscription type: standard (Annual: Fixed capacity subscription from Red Hat)
? Authentication method: Service account
? Service account file: /Users/jdgray/gitlab/temps/sda-ccs-1/jdg-ccs-1-key.json
? Multiple AZ: No
? Secure boot support for Shielded VMs: No
? OpenShift version: 4.17.0
? Enable autoscaling: No
? Compute nodes: 2
? Private cluster (optional): Yes
? Install into an existing VPC (optional): [? for help] (y/N) 

In the interactive flow where I've already indicated that I'm using PSC it asks me if I want a private cluster and if I want to install into an existing VPC. Both of these prompts have a "right" and "wrong" answer in this case, and the default option is the incorrect one.

If managing the flow is too complex then it might not be worth pursuing, because at least if the user chooses wrong they receive quick feedback on their mistake.

@ckandag
Copy link
Collaborator Author

ckandag commented Oct 22, 2024

My comment is still mostly the same

ocm create cluster -i --dry-run=true --ccs  --provider=gcp --region=us-east1 --compute-machine-type=n2-standard-4 --psc-subnet=jdg-21-tf2d8-worker-subnet jjjj
? Subscription type: standard (Annual: Fixed capacity subscription from Red Hat)
? Authentication method: Service account
? Service account file: /Users/jdgray/gitlab/temps/sda-ccs-1/jdg-ccs-1-key.json
? Multiple AZ: No
? Secure boot support for Shielded VMs: No
? OpenShift version: 4.17.0
? Enable autoscaling: No
? Compute nodes: 2
? Private cluster (optional): Yes
? Install into an existing VPC (optional): [? for help] (y/N) 

In the interactive flow where I've already indicated that I'm using PSC it asks me if I want a private cluster and if I want to install into an existing VPC. Both of these prompts have a "right" and "wrong" answer in this case, and the default option is the incorrect one.

If managing the flow is too complex then it might not be worth pursuing, because at least if the user chooses wrong they receive quick feedback on their mistake.

The interactive flow I think has value and does work when user tries it all interactively.
An edge case with a mix of both non interactive and interactive flags which is not working.
which ideally that too should but given the main flows work in both interactive anad non-interactive mode.
I will look into fixing the above mixed case if its not too complicated.

Signed-off-by: Chaitanya Kandagatla <ckandaga@redhat.com>
Copy link
Collaborator

@renan-campos renan-campos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@ckandag ckandag merged commit db35759 into openshift-online:main Oct 22, 2024
4 checks passed
@ckandag ckandag deleted the psc-ux branch October 22, 2024 20:17
ckandag added a commit that referenced this pull request Oct 25, 2024
-8b70707 Release v0.1.76 (#674)
-2ef1e1d Update Konflux references to 67f0290 (#676)
-c63247f Show all WIF configs in interactive dropdown (#677)
-f1d29bc adding interactive move for WifConfig creation (#675)
-1382a6c Require provider, do not default to AWS, and check for provider-specific flags (#678)
-d0b58b4 Do not default GCP authentication type (#679)
-db35759 improve psc cli UX (#681)
-862b072 OCM-11993 | Describe cluster shows WifConfig data (#683)
-4053505 Add PSC-XPN to cluster  description (#685)
-0c456f8 OCM-10728 | interface improvements (#686)
-1e27ded Add more descriptions to WIF resources (#684)
-cdf6466 update filenames in konflux release  container (#687)
-010573f remove version from shasum (#688)
@ckandag ckandag mentioned this pull request Oct 25, 2024
renan-campos pushed a commit that referenced this pull request Oct 25, 2024
-8b70707 Release v0.1.76 (#674)
-2ef1e1d Update Konflux references to 67f0290 (#676)
-c63247f Show all WIF configs in interactive dropdown (#677)
-f1d29bc adding interactive move for WifConfig creation (#675)
-1382a6c Require provider, do not default to AWS, and check for provider-specific flags (#678)
-d0b58b4 Do not default GCP authentication type (#679)
-db35759 improve psc cli UX (#681)
-862b072 OCM-11993 | Describe cluster shows WifConfig data (#683)
-4053505 Add PSC-XPN to cluster  description (#685)
-0c456f8 OCM-10728 | interface improvements (#686)
-1e27ded Add more descriptions to WIF resources (#684)
-cdf6466 update filenames in konflux release  container (#687)
-010573f remove version from shasum (#688)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants