astro-cli icon indicating copy to clipboard operation
astro-cli copied to clipboard

worker queue create

Open jemishp opened this issue 3 years ago • 1 comments

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 test before taking out of draft
  • [x] Ran make lint before 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

jemishp avatar Aug 04 '22 18:08 jemishp

Codecov Report

Merging #674 (8264ab8) into main (f33e5e5) will increase coverage by 0.32%. The diff coverage is 100.00%.

Impacted file tree graph

@@            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 data Powered by Codecov. Last update f33e5e5...8264ab8. Read the comment docs.

codecov[bot] avatar Aug 04 '22 18:08 codecov[bot]

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 image

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 avatar Aug 29 '22 20:08 jemishp

@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

sunkickr avatar Aug 30 '22 13:08 sunkickr