Skip to content

Commit 1d2eec7

Browse files
sunkickrkushalmalani
sunkickr
and
kushalmalani
authored
Fix issues from bug bash (#1460)
* Fix issues from bug bash * refactor code * Update cloud/deploy/deploy.go Co-authored-by: kushalmalani <kushal@astronomer.io> * fix test * fix test --------- Co-authored-by: kushalmalani <kushal@astronomer.io>
1 parent 1823da2 commit 1d2eec7

File tree

4 files changed

+68
-29
lines changed

4 files changed

+68
-29
lines changed

‎cloud/deploy/deploy.go

+38-26
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,10 @@ var (
7070
)
7171

7272
var (
73-
errDagsParseFailed = errors.New("your local DAGs did not parse. Fix the listed errors or use `astro deploy [deployment-id] -f` to force deploy") //nolint:revive
74-
envFileMissing = errors.New("Env file path is incorrect: ") //nolint:revive
75-
errCiCdEnforcementUpdate = errors.New("cannot update dag deploy since ci/cd enforcement is enabled for this deployment. Please use API Tokens or API Keys instead")
73+
errDagsParseFailed = errors.New("your local DAGs did not parse. Fix the listed errors or use `astro deploy [deployment-id] -f` to force deploy") //nolint:revive
74+
envFileMissing = errors.New("Env file path is incorrect: ") //nolint:revive
75+
errCiCdEnforcementUpdate = errors.New("cannot update dag deploy since ci/cd enforcement is enabled for this deployment. Please use API Tokens or API Keys instead")
76+
errImageDeployNoPriorDags = errors.New("cannot do image only deploy with no prior DAGs deployed. Please deploy DAGs to your deployment first")
7677
)
7778

7879
var (
@@ -83,16 +84,17 @@ var (
8384
)
8485

8586
type deploymentInfo struct {
86-
deploymentID string
87-
namespace string
88-
deployImage string
89-
currentVersion string
90-
organizationID string
91-
workspaceID string
92-
webserverURL string
93-
dagDeployEnabled bool
94-
deploymentType string
95-
cicdEnforcement bool
87+
deploymentID string
88+
namespace string
89+
deployImage string
90+
currentVersion string
91+
organizationID string
92+
workspaceID string
93+
webserverURL string
94+
deploymentType string
95+
desiredDagTarballVersion string
96+
dagDeployEnabled bool
97+
cicdEnforcement bool
9698
}
9799

98100
type InputDeploy struct {
@@ -232,6 +234,15 @@ func Deploy(deployInput InputDeploy, client astro.Client, coreClient astrocore.C
232234
return nil
233235
}
234236

237+
if deployInput.Image {
238+
if !deployInfo.dagDeployEnabled {
239+
return fmt.Errorf(enableDagDeployMsg, deployInfo.deploymentID) //nolint
240+
}
241+
if deployInfo.desiredDagTarballVersion == "" {
242+
return errImageDeployNoPriorDags
243+
}
244+
}
245+
235246
deploymentURL, err := deployment.GetDeploymentURL(deployInfo.deploymentID, deployInfo.workspaceID)
236247
if err != nil {
237248
return err
@@ -372,23 +383,12 @@ func Deploy(deployInput InputDeploy, client astro.Client, coreClient astrocore.C
372383
return err
373384
}
374385
} else {
375-
if !deployInfo.dagDeployEnabled {
376-
return fmt.Errorf(enableDagDeployMsg, deployInfo.deploymentID) //nolint
377-
}
378386
fmt.Println("Image Deploy only. Skipping deploying DAG...")
379387
}
380388
}
381389
// finish deploy
382390
if deployInput.Image {
383-
coreDeployment, err := deployment.CoreGetDeployment(deployInfo.workspaceID, deployInfo.organizationID, deployInfo.deploymentID, coreClient)
384-
if err != nil {
385-
return err
386-
}
387-
if coreDeployment.CurrentDagTarballVersion != nil {
388-
dagTarballVersion = *coreDeployment.CurrentDagTarballVersion
389-
} else {
390-
dagTarballVersion = ""
391-
}
391+
dagTarballVersion = deployInfo.desiredDagTarballVersion
392392
}
393393
err = updateDeploy(deployID, deployInfo.deploymentID, deployInfo.organizationID, dagTarballVersion, deployInfo.dagDeployEnabled, coreClient)
394394
if err != nil {
@@ -430,6 +430,17 @@ func getDeploymentInfo(deploymentID, wsID, deploymentName string, prompt bool, c
430430
if err != nil {
431431
return deploymentInfo{}, err
432432
}
433+
coreDeployment, err := deployment.CoreGetDeployment(currentDeployment.Workspace.ID, currentDeployment.Workspace.OrganizationID, currentDeployment.ID, coreClient)
434+
if err != nil {
435+
return deploymentInfo{}, err
436+
}
437+
var desiredDagTarballVersion string
438+
if coreDeployment.DesiredDagTarballVersion != nil {
439+
desiredDagTarballVersion = *coreDeployment.DesiredDagTarballVersion
440+
} else {
441+
desiredDagTarballVersion = ""
442+
}
443+
433444
return deploymentInfo{
434445
currentDeployment.ID,
435446
currentDeployment.ReleaseName,
@@ -438,8 +449,9 @@ func getDeploymentInfo(deploymentID, wsID, deploymentName string, prompt bool, c
438449
currentDeployment.Workspace.OrganizationID,
439450
currentDeployment.Workspace.ID,
440451
currentDeployment.DeploymentSpec.Webserver.URL,
441-
currentDeployment.DagDeployEnabled,
442452
currentDeployment.Type,
453+
desiredDagTarballVersion,
454+
currentDeployment.DagDeployEnabled,
443455
currentDeployment.APIKeyOnlyDeployments,
444456
}, nil
445457
}

‎cloud/deploy/deploy_test.go

+24-1
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ var (
3030
ws = "test-ws-id"
3131
dagTarballVersionTest = "test-version"
3232
dagsUploadTestURL = "test-url"
33+
deploymentID = "test-id"
3334
createDeployResponse = astrocore.CreateDeployResponse{
3435
HTTPResponse: &http.Response{
3536
StatusCode: 200,
@@ -74,6 +75,20 @@ var (
7475
IsDagDeployEnabled: false,
7576
},
7677
}
78+
mockCoreDeploymentResponse = []astrocore.Deployment{
79+
{
80+
Id: deploymentID,
81+
Status: "HEALTHY",
82+
},
83+
}
84+
mockListDeploymentsResponse = astrocore.ListDeploymentsResponse{
85+
HTTPResponse: &http.Response{
86+
StatusCode: 200,
87+
},
88+
JSON200: &astrocore.DeploymentsPaginated{
89+
Deployments: mockCoreDeploymentResponse,
90+
},
91+
}
7792
)
7893

7994
func TestDeployWithoutDagsDeploySuccess(t *testing.T) {
@@ -95,6 +110,7 @@ func TestDeployWithoutDagsDeploySuccess(t *testing.T) {
95110
mockClient := new(astro_mocks.Client)
96111

97112
mockCoreClient.On("GetDeploymentWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&deploymentResponse, nil).Times(4)
113+
mockCoreClient.On("ListDeploymentsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&mockListDeploymentsResponse, nil).Times(1)
98114
mockClient.On("ListDeployments", org, ws).Return([]astro.Deployment{{ID: "test-id", Workspace: astro.Workspace{ID: ws}}}, nil).Once()
99115
mockCoreClient.On("GetDeploymentOptionsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&getDeploymentOptionsResponse, nil).Times(5)
100116
mockCoreClient.On("CreateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&createDeployResponse, nil).Times(5)
@@ -201,7 +217,7 @@ func TestDeployOnCiCdEnforcedDeployment(t *testing.T) {
201217
canCiCdDeploy = func(astroAPIToken string) bool {
202218
return false
203219
}
204-
220+
mockCoreClient.On("ListDeploymentsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&mockListDeploymentsResponse, nil).Times(1)
205221
mockClient.On("ListDeployments", org, ws).Return([]astro.Deployment{{ID: "test-id", Workspace: astro.Workspace{ID: ws}, DagDeployEnabled: true, APIKeyOnlyDeployments: true}}, nil).Once()
206222

207223
err := Deploy(deployInput, mockClient, mockCoreClient)
@@ -237,6 +253,7 @@ func TestDeployWithDagsDeploySuccess(t *testing.T) {
237253

238254
mockCoreClient.On("GetDeploymentWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&deploymentResponse, nil).Times(5)
239255
mockClient.On("ListDeployments", org, ws).Return([]astro.Deployment{{ID: "test-id", Workspace: astro.Workspace{ID: ws}, DagDeployEnabled: true}}, nil).Times(2)
256+
mockCoreClient.On("ListDeploymentsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&mockListDeploymentsResponse, nil).Times(2)
240257
mockCoreClient.On("GetDeploymentOptionsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&getDeploymentOptionsResponse, nil).Times(7)
241258
mockCoreClient.On("CreateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&createDeployResponse, nil).Times(7)
242259
mockCoreClient.On("UpdateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&updateDeployResponse, nil).Times(7)
@@ -393,6 +410,7 @@ func TestDagsDeploySuccess(t *testing.T) {
393410

394411
mockCoreClient.On("GetDeploymentOptionsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&getDeploymentOptionsResponse, nil).Times(3)
395412
mockClient.On("ListDeployments", mock.Anything, mock.Anything).Return(mockDeplyResp, nil).Times(5)
413+
mockCoreClient.On("ListDeploymentsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&mockListDeploymentsResponse, nil).Times(5)
396414
mockCoreClient.On("CreateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&createDeployResponse, nil).Times(5)
397415
mockCoreClient.On("UpdateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&updateDeployResponse, nil).Times(5)
398416

@@ -488,6 +506,7 @@ func TestNoDagsDeploy(t *testing.T) {
488506
}
489507

490508
mockClient.On("ListDeployments", mock.Anything, mock.Anything).Return(mockDeplyResp, nil).Times(1)
509+
mockCoreClient.On("ListDeploymentsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&mockListDeploymentsResponse, nil).Times(1)
491510
mockCoreClient.On("CreateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&createDeployResponse, nil).Times(1)
492511

493512
deployInput := InputDeploy{
@@ -553,6 +572,7 @@ func TestDagsDeployFailed(t *testing.T) {
553572
Dags: true,
554573
}
555574
mockClient.On("ListDeployments", mock.Anything, mock.Anything).Return(mockDeplyResp, nil).Times(3)
575+
mockCoreClient.On("ListDeploymentsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&mockListDeploymentsResponse, nil).Times(3)
556576
mockCoreClient.On("GetDeploymentOptionsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&getDeploymentOptionsResponse, nil).Times(2)
557577
mockCoreClient.On("CreateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&createDeployResponse, nil).Times(3)
558578

@@ -633,6 +653,7 @@ func TestDeployFailure(t *testing.T) {
633653
testUtil.InitTestConfig(testUtil.CloudPlatform)
634654
mockClient := new(astro_mocks.Client)
635655
mockClient.On("ListDeployments", org, ws).Return(mockDeplyResp, nil).Times(2)
656+
mockCoreClient.On("ListDeploymentsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&mockListDeploymentsResponse, nil).Times(3)
636657
mockCoreClient.On("CreateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&createDeployResponse, nil).Times(2)
637658
mockCoreClient.On("GetDeploymentOptionsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&getDeploymentOptionsResponse, nil).Once()
638659

@@ -740,6 +761,7 @@ func TestDeployMonitoringDAGNonHosted(t *testing.T) {
740761

741762
mockCoreClient.On("GetDeploymentOptionsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&getDeploymentOptionsResponse, nil).Times(3)
742763
mockClient.On("ListDeployments", mock.Anything, mock.Anything).Return(mockDeplyResp, nil).Times(4)
764+
mockCoreClient.On("ListDeploymentsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&mockListDeploymentsResponse, nil).Times(4)
743765
mockCoreClient.On("CreateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&createDeployResponse, nil).Times(4)
744766
mockCoreClient.On("UpdateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&updateDeployResponse, nil).Times(4)
745767

@@ -844,6 +866,7 @@ func TestDeployNoMonitoringDAGHosted(t *testing.T) {
844866

845867
mockCoreClient.On("GetDeploymentOptionsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&getDeploymentOptionsResponse, nil).Times(3)
846868
mockClient.On("ListDeployments", mock.Anything, mock.Anything).Return(mockDeplyResp, nil).Times(4)
869+
mockCoreClient.On("ListDeploymentsWithResponse", mock.Anything, mock.Anything, mock.Anything).Return(&mockListDeploymentsResponse, nil).Times(4)
847870
mockCoreClient.On("CreateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&createDeployResponse, nil).Times(4)
848871
mockCoreClient.On("UpdateDeployWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&updateDeployResponse, nil).Times(4)
849872

‎cloud/deployment/deployment.go

+4
Original file line numberDiff line numberDiff line change
@@ -696,6 +696,10 @@ func Update(deploymentID, label, ws, description, deploymentName, dagDeploy, exe
696696
}
697697
}
698698

699+
// determine dagDeploy enabled/disabled
700+
if dagDeploy == "" {
701+
deploymentUpdate.DagDeployEnabled = currentDeployment.DagDeployEnabled
702+
}
699703
if dagDeploy == "enable" {
700704
if currentDeployment.DagDeployEnabled {
701705
fmt.Println("\nDAG deploys are already enabled for this Deployment. Your DAGs will continue to run as scheduled.")

‎cloud/deployment/deployment_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -2605,7 +2605,7 @@ func TestUpdate(t *testing.T) { //nolint
26052605
ClusterID: "",
26062606
Label: "",
26072607
Description: "",
2608-
DagDeployEnabled: false,
2608+
DagDeployEnabled: true,
26092609
DeploymentSpec: astro.DeploymentCreateSpec{
26102610
Executor: KubeExecutor,
26112611
Scheduler: astro.Scheduler{AU: 5, Replicas: 3},
@@ -2678,7 +2678,7 @@ func TestUpdate(t *testing.T) { //nolint
26782678
ClusterID: "",
26792679
Label: "",
26802680
Description: "",
2681-
DagDeployEnabled: false,
2681+
DagDeployEnabled: true,
26822682
DeploymentSpec: astro.DeploymentCreateSpec{
26832683
Executor: CeleryExecutor,
26842684
Scheduler: astro.Scheduler{AU: 5, Replicas: 3},

0 commit comments

Comments
 (0)