fix dag deploy updating on accident #1523
Conversation
sunkickr
requested review from
neel-astro,
kushalmalani and
vandyliu
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1523 +/- ##
==========================================
- Coverage 85.90% 85.89% -0.02%
==========================================
Files 112 112
Lines 14847 14848 +1
==========================================
- Hits 12755 12754 -1
- Misses 1254 1256 +2
Partials 838 838 ☔ View full report in Codecov by Sentry. |
| @@ -69,6 +69,7 @@ var ( | |||
| tickNum = 10 | |||
| timeoutNum = 180 | |||
| dedicatedDeploymentRequest = astroplatformcore.UpdateDedicatedDeploymentRequest{} | |||
| dagDeployEnabled bool | |||
There was a problem hiding this comment.
what was the reasoning behind putting it up here, it seems to fit the style of code the way we had it before
There was a problem hiding this comment.
We need it exist outside of the function so we can assert that it is the right value in the test
cloud/deployment/deployment.go
Outdated
| @@ -768,6 +767,10 @@ func Update(deploymentID, name, ws, description, deploymentName, dagDeploy, exec | |||
| dagDeployEnabled = false | |||
| } | |||
|
|
|||
| if dagDeploy == "" { | |||
There was a problem hiding this comment.
| if dagDeploy == "" { | |
| else if dagDeploy == "" { |
|
Can you add some screenshots for this. This bug fix is critical |
| @@ -1288,6 +1288,7 @@ func TestUpdate(t *testing.T) { //nolint | |||
| // success with hybrid type | |||
| err := Update("", "", ws, "", "", "", CeleryExecutor, "", "", "", "", "", "", "", 0, 0, workerQueueRequest, hybridQueueList, newEnvironmentVariables, true, mockCoreClient, mockPlatformCoreClient) | |||
There was a problem hiding this comment.
Can we add a comment up here w.r.t to what are we updating?
Eventually would want to get rid of that huge list of arguments and have defined input type, but we can do that later.
Description
When running the command
astro deployment updatedag deploy is always turned off. This is because the case for dag deploy not changing didn't exist.Added an assert to the update test to test for this bug.
Related #XXX
🧪 Functional Testing
📸 Screenshots
screenshot of updating the description of both a deployment with dag deploy enabled and disabled. Showing that in both cases dag deploy stays the same:
📋 Checklist
make testbefore taking out of draftmake lintbefore taking out of draft