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.

Sign up for GitHub

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

Jump to bottom

fix dag deploy updating on accident #1523

Merged
merged 6 commits into from
Feb 2, 2024
Merged

fix dag deploy updating on accident #1523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
da84816
fix dag deploy updating on accident
sunkickr Feb 1, 2024
c32e138
add assert to test
sunkickr Feb 1, 2024
c3580cc
fix lint
sunkickr Feb 1, 2024
e67dd79
update to switch
sunkickr Feb 2, 2024
6c0b851
update comments
sunkickr Feb 2, 2024
a0ebd2d
fix lint
sunkickr Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions cloud/deployment/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ var (
tickNum = 10
timeoutNum = 180
dedicatedDeploymentRequest = astroplatformcore.UpdateDedicatedDeploymentRequest{}
dagDeployEnabled bool
Copy link
Contributor

Choose a reason for hiding this comment

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

what was the reasoning behind putting it up here, it seems to fit the style of code the way we had it before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need it exist outside of the function so we can assert that it is the right value in the test

)

func newTableOut() *printutil.Table {
Expand Down Expand Up @@ -219,7 +220,6 @@ func Logs(deploymentID, ws, deploymentName string, warnLogs, errorLogs, infoLogs
func Create(name, workspaceID, description, clusterID, runtimeVersion, dagDeploy, executor, cloudProvider, region, schedulerSize, highAvailability, cicdEnforcement, defaultTaskPodCpu, defaultTaskPodMemory, resourceQuotaCpu, resourceQuotaMemory string, deploymentType astroplatformcore.DeploymentType, schedulerAU, schedulerReplicas int, platformCoreClient astroplatformcore.CoreClient, coreClient astrocore.CoreClient, waitForStatus bool) error { //nolint
var organizationID string
var currentWorkspace astrocore.Workspace
var dagDeployEnabled bool

c, err := config.GetCurrentContext()
if err != nil {
Expand Down Expand Up @@ -745,8 +745,8 @@ func Update(deploymentID, name, ws, description, deploymentName, dagDeploy, exec
}
}
// determine isDagDeployEnabled
var dagDeployEnabled bool
if dagDeploy == enable {
switch dagDeploy {
case enable:
if currentDeployment.IsDagDeployEnabled {
fmt.Println("\nDAG deploys are already enabled for this Deployment. Your DAGs will continue to run as scheduled.")
return nil
Expand All @@ -755,7 +755,7 @@ func Update(deploymentID, name, ws, description, deploymentName, dagDeploy, exec
fmt.Printf("\nYou enabled DAG-only deploys for this Deployment. Running tasks are not interrupted but new tasks will not be scheduled." +
"\nRun `astro deploy --dags` to complete enabling this feature and resume your DAGs. It may take a few minutes for the Airflow UI to update..\n\n")
dagDeployEnabled = true
} else if dagDeploy == disable {
case disable:
if !currentDeployment.IsDagDeployEnabled {
fmt.Println("\nDAG-only deploys is already disabled for this deployment.")
return nil
Expand All @@ -766,8 +766,9 @@ func Update(deploymentID, name, ws, description, deploymentName, dagDeploy, exec
confirmWithUser = true
}
dagDeployEnabled = false
case "":
dagDeployEnabled = currentDeployment.IsDagDeployEnabled
}

configOption, err := GetDeploymentOptions("", astrocore.GetDeploymentOptionsParams{}, coreClient)
if err != nil {
return err
Expand Down
18 changes: 11 additions & 7 deletions cloud/deployment/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,15 +1285,16 @@ func TestUpdate(t *testing.T) { //nolint
// Mock user input for deployment name
defer testUtil.MockUserInput(t, "1")()

// success with hybrid type
// success with hybrid type in this test nothing is being change just ensuring that dag deploy stays true. Addtionally no deployment id/name is given so user input is needed to select one
err := Update("", "", ws, "", "", "", CeleryExecutor, "", "", "", "", "", "", "", 0, 0, workerQueueRequest, hybridQueueList, newEnvironmentVariables, true, mockCoreClient, mockPlatformCoreClient)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comments

assert.NoError(t, err)
assert.Equal(t, deploymentResponse.JSON200.IsDagDeployEnabled, dagDeployEnabled)

// Mock user input for deployment name
// defer testUtil.MockUserInput(t, "1")()
defer testUtil.MockUserInput(t, "y")()

// kubernetes executor
// success updating the kubernetes executor on hybrid type. deployment name is given
err = Update("test-id-1", "", ws, "", "", "", KubeExecutor, "", "", "", "", "", "", "", 0, 0, workerQueueRequest, hybridQueueList, newEnvironmentVariables, false, mockCoreClient, mockPlatformCoreClient)
assert.NoError(t, err)

Expand All @@ -1305,14 +1306,15 @@ func TestUpdate(t *testing.T) { //nolint
// defer testUtil.MockUserInput(t, "1")()
defer testUtil.MockUserInput(t, "y")()

// success with standard type
// success with standard type and deployment name input and dag deploy stays the same
err = Update("test-id-1", "", ws, "", "", "", CeleryExecutor, "", "", "", "", "", "", "", 0, 0, workerQueueRequest, hybridQueueList, newEnvironmentVariables, false, mockCoreClient, mockPlatformCoreClient)
assert.NoError(t, err)
assert.Equal(t, deploymentResponse.JSON200.IsDagDeployEnabled, dagDeployEnabled)

// Mock user input for deployment name
defer testUtil.MockUserInput(t, "y")()

// kubernetes executor
// success updating to kubernetes executor on standard type
err = Update("test-id-1", "", ws, "", "", "", KubeExecutor, "", "", "", "", "", "", "", 0, 0, workerQueueRequest, hybridQueueList, newEnvironmentVariables, false, mockCoreClient, mockPlatformCoreClient)
assert.NoError(t, err)

Expand All @@ -1322,14 +1324,16 @@ func TestUpdate(t *testing.T) { //nolint
// Mock user input for deployment name
// defer testUtil.MockUserInput(t, "1")()

// success with dedicated type
// success with dedicated type no changes made asserts that dag deploy stays the same
err = Update("test-id-1", "", ws, "", "", "", CeleryExecutor, "", "", "", "", "", "", "", 0, 0, workerQueueRequest, hybridQueueList, newEnvironmentVariables, false, mockCoreClient, mockPlatformCoreClient)
assert.NoError(t, err)
assert.Equal(t, deploymentResponse.JSON200.IsDagDeployEnabled, dagDeployEnabled)

// Mock user input for deployment name
defer testUtil.MockUserInput(t, "y")()

// kubernetes executor
err = Update("test-id-1", "", ws, "", "", "", "", "", "", "", "", "", "", "", 0, 0, workerQueueRequest, hybridQueueList, newEnvironmentVariables, false, mockCoreClient, mockPlatformCoreClient)
// success with dedicated updating to kubernetes executor
err = Update("test-id-1", "", ws, "", "", "", KubeExecutor, "", "", "", "", "", "", "", 0, 0, workerQueueRequest, hybridQueueList, newEnvironmentVariables, false, mockCoreClient, mockPlatformCoreClient)
assert.NoError(t, err)
mockCoreClient.AssertExpectations(t)
mockPlatformCoreClient.AssertExpectations(t)
Expand Down