worker queue create
Description
This PR adds a command to create a new worker queue for a deployment discussed in #659. Looking to get early general feedback, specifically on how much validation and extra safety checks the CLI should do for a better UX.
🎟 Issue(s)
Related #659
🧪 Functional Testing
create a non default worker-queue
$astro deployment worker-queue create -n myQueue --isDefault=false
Select which Deployment you want to update
# DEPLOYMENT NAME RELEASE NAME DEPLOYMENT ID
1 neel-dev-test advanced-precession-1762 cl60i43ye406311hvr3amdaxvy
2 jp-test devoid-declination-6370 cl6omxtrn1901511jwqqjm15tkg
3 worker-queue-test telescopic-intensity-5645 cl6p6330i368002cripuzq1l8u
> 2
cl6omxtrn1901511jwqqjm15tkg
NAME NAMESPACE WORKSPACE ID CLUSTER ID DEPLOYMENT ID DOCKER IMAGE TAG RUNTIME VERSION
jp-test devoid-declination-6370 cl0v1p6lc728255byzyfs7lw21 cl63a5mmx009z0sxfdw90248d cl6omxtrn1901511jwqqjm15tkg 5.0.6 5.0.6 (based on Airflow 2.3.3)
Successfully updated Deployment
worker-queue myQueue for cl6omxtrn1901511jwqqjm15tkg in cl0v1p6lc728255byzyfs7lw21 workspace created
errors when 2 default queues are being requested
$astro deployment worker-queue create -n default2 --isDefault=true
Select which Deployment you want to update
# DEPLOYMENT NAME RELEASE NAME DEPLOYMENT ID
1 neel-dev-test advanced-precession-1762 cl60i43ye406311hvr3amdaxvy
2 jp-test devoid-declination-6370 cl6omxtrn1901511jwqqjm15tkg
3 worker-queue-test telescopic-intensity-5645 cl6p6330i368002cripuzq1l8u
> 2
cl6omxtrn1901511jwqqjm15tkg
Error: Cannot connect to Astronomer. Try to log in with astro login or check your internet connection and user permissions.
Details: A single default worker queue must be included in the worker queue list (found 2)
errors when existing queue is being modified
errors when input values for min, max, concurrency are out of bounds
errors when any api call made to get deployments, get default values, etc. fails
📋 Checklist
- [x] Rebased from the main (or release if patching) branch (before testing)
- [x] Ran
make testbefore taking out of draft - [x] Ran
make lintbefore taking out of draft - [x] Added/updated applicable tests
- [ ] Tested against Astro-API (if necessary).
- [ ] Tested against Houston-API and Astronomer (if necessary).
- [ ] Communicated to/tagged owners of respective clients potentially impacted by these changes.
- [ ] Updated any related documentation
Codecov Report
Merging #674 (8264ab8) into main (f33e5e5) will increase coverage by
0.32%. The diff coverage is100.00%.
@@ Coverage Diff @@
## main #674 +/- ##
==========================================
+ Coverage 85.92% 86.25% +0.32%
==========================================
Files 93 95 +2
Lines 7491 7653 +162
==========================================
+ Hits 6437 6601 +164
+ Misses 649 648 -1
+ Partials 405 404 -1
| Impacted Files | Coverage Δ | |
|---|---|---|
| astro-client/astro.go | 100.00% <100.00%> (ø) |
|
| cloud/deployment/deployment.go | 89.97% <100.00%> (+0.05%) |
:arrow_up: |
| cloud/deployment/workerqueue/workerqueue.go | 100.00% <100.00%> (ø) |
|
| cmd/cloud/deployment.go | 91.54% <100.00%> (+0.03%) |
:arrow_up: |
| cmd/cloud/deployment_workerqueue.go | 100.00% <100.00%> (ø) |
|
| cmd/cloud/workspace.go | 94.11% <0.00%> (+3.92%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update f33e5e5...8264ab8. Read the comment docs.
This PR looks good. Everything seems to be working right just requesting a few copy changes
Also not sure if this extra deployment id is coming from code in this PR but I think we should remove it
Why is a table with information about the deployment being printed? If we are going to print a table. it would be more helpful if it was a table containing information about the worker queue just created
@sunkickr the extra deploymentID is being printed from this line NOT part of this PR.
The table print out is coming from deployment.Update() here also NOT part of this PR.
Should we create a separate issue/s to change deployment.Update() and deployment.GetDeployment()? Or can we handle it in #639 when we remove deprecate worker-AU?
@jemishp yeah we should probably refactor deployment update if the function is going to be part of the worker queue commands. The deployment update command needs the table but the worker queue command does not. We probably need to abstract the table print out of deployment.Update. Up to you when to do this. I think it can be done when updating worker-au. up to you
