Added the support for clusterID for the pre-created namespaces flows in CLI #1907
Added the support for clusterID for the pre-created namespaces flows in CLI #1907rujhan-arora-astronomer merged 5 commits intogh_7572from
Conversation
rujhan-arora-astronomer
requested review from
jeremybeard,
kushalmalani and
neel-astro
as code owners
|
@neel-astro I think the CodeOwners file is correct, but it didn't tag the mentioned folks. Do I need to change something? |
rujhan-arora-astronomer
requested review from
karankhanchandani and
pgvishnuram
|
@rujhan-arora-astronomer It won't apply until the codeowners change is merged |
cmd/software/deployment.go
Outdated
| deploymentCreateExample = ` | ||
| # Create new deployment with Celery executor (default: celery without params). | ||
| $ astro deployment create --label=new-deployment-name --executor=celery | ||
| $ astro deployment create --label=new-deployment-name --executor=celery --cluster-id=123(mandatory for Software release 1.0.0 and above) |
There was a problem hiding this comment.
do we need to put them on brackets ??
There was a problem hiding this comment.
They don't need to add in brackets. Where do you see it?
There was a problem hiding this comment.
I think what Vishnu is saying that in the cli, you see (mandatory for Software release 1.0.0 and above) in the examples section of the command. Do we need it there? We can add this information (cluster ID is mandatory) in the helper of this command.
There was a problem hiding this comment.
since the cli itself makes the field as required explicity adding in the command examples might not make sense
cmd/software/deployment.go
Outdated
| if houstonVersion >= "1.0.0" { | ||
| cmd.Flags().StringVarP(&clusterID, "cluster-id", "", "", "Set cluster ID to create deployment in (mandatory for Software release 1.0.0 and above)") | ||
| _ = cmd.MarkFlagRequired("cluster-id") | ||
| } |
There was a problem hiding this comment.
with this if we set a clusterId and any create deployment we initiate it will use the set clusterId right
There was a problem hiding this comment.
Yes, this clusterId will be used for that create call
There was a problem hiding this comment.
do workspace admin will able to make this call like set clusterId
There was a problem hiding this comment.
Anyone can pass the cluster ID in this command. We don't have RBACs at the CLI level.
karankhanchandani
left a comment
karankhanchandani
left a comment
There was a problem hiding this comment.
Thanks for the PR. LGTM. Can you please resolve the conversations before merging?
|
4 participants
Description
Currently, the CLI fetches the list of available pre-created namespaces and applies filters based on the Houston configmap. This approach is problematic for 1.0 because it doesn't account for feature flags or specific configurations that may be enabled on a per-cluster basis.
As a result, a user might see a list of pre-created namespaces that is not accurate for their target environment.
This PR resolves this issue by ensuring that whenever the CLI requests a list of pre-created namespaces, it passes the relevant clusterId ID. The backend API can then use this ID to evaluate cluster-specific feature flags and return a precise, filtered list of pre-created namespaces that are actually available for that cluster.
🎟 Issue(s)
Related https://github.com/astronomer/issues/issues/7590
🧪 Functional Testing
Locally
Unit tests
📸 Screenshots
📋 Checklist
make testbefore taking out of draftmake lintbefore taking out of draft