AF3 Astro Executor support for deployment create / update by jaketf · Pull Request #1854 · astronomer/astro-cli

jaketf · 2025-05-21T22:23:03Z

Description

Describe the purpose of this pull request.

Adds support to astro deployment [create|update] for AF 3 with Astro Executor

🎟 Issue(s)

Fixes #1852
Related #1853

🧪 Functional Testing

List the functional testing steps to confirm this feature or fix.
successful deployment creation of minimal Hosted AF3 deployment w/ astro executor

make build
./astro deployment create -v cm4sauxuo03ig01rfux2aun5d -v 3.0-1 -e ASTRO -n test-astro-exec-from-cli-2
Current Workspace: jake


Please select a Region for your Deployment:
 #     REGION            
 1     westus2           
 2     eastus2           
 3     westeurope        
 4     australiaeast     

> 1
 NAME                           NAMESPACE                   CLUSTER     CLOUD PROVIDER     REGION      DEPLOYMENT ID                 RUNTIME VERSION                    DAG DEPLOY ENABLED     CI-CD ENFORCEMENT     DEPLOYMENT TYPE     
 test-astro-exec-from-cli-2     explosive-parallax-1197     N/A         AZURE              westus2     cmbvbhl2f02pj01l6m1tv8g8q     3.0-1 (based on Airflow 3.0.0)     true                   false                 STANDARD            

 Successfully created Deployment: test-astro-exec-from-cli-2
 Deployment can be accessed at the following URLs 

 Deployment Dashboard: cloud.astronomer-dev.io/cm8s7ergj00vm01ovrdxw26u8/deployments/cmbvbhl2f02pj01l6m1tv8g8q/overview
 Airflow Dashboard: clztuyh0a01z701mwxjahsza2.astronomer-dev.run/d1tv8g8q?orgId=org_bql5Hn1KELMRNPoM

📸 Screenshots

Add screenshots to illustrate the validity of these changes.

📋 Checklist

Rebased from the main (or release if patching) branch (before testing)
Ran make test before taking out of draft
Ran make lint before taking out of draft
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

astro-client-core/api.gen.go

cloud/deployment/deployment_test.go

feluelle · 2025-06-16T07:16:47Z

cloud/deployment/fromfile/fromfile.go

+	// check if deployment is using Airflow 3 by validating runtime version
+	isAirflow3 := airflowversions.IsAirflow3(deploymentFromFile.Deployment.Configuration.RunTimeVersion)
+	if !isValidExecutor(deploymentFromFile.Deployment.Configuration.Executor, isAirflow3) {


hmm, I think it would be nicer if you could pass deploymentFromFile.Deployment.Configuration.RunTimeVersion instead of isAirflow3 to the isValidExecutor, from looking at just the call.

Suggested change

// check if deployment is using Airflow 3 by validating runtime version

isAirflow3 := airflowversions.IsAirflow3(deploymentFromFile.Deployment.Configuration.RunTimeVersion)

if !isValidExecutor(deploymentFromFile.Deployment.Configuration.Executor, isAirflow3) {

if !isValidExecutor(deploymentFromFile.Deployment.Configuration.Executor, deploymentFromFile.Deployment.Configuration.RunTimeVersion) {

..and check for airflow 3 compatibility within valid executor determination.

@jaketf, did you revert this? Was that intentional? I'm okay with that if you think it's better.

Ah, you changed this in cmd/cloud/deployment.go but not in this module.

cmd/airflow_test.go

cmd/cloud/deployment.go

.pre-commit-config.yaml

go.mod

for more information, see https://pre-commit.ci

this commit is just to outline explicitly the changes we need to make in the astro core platform API. It can be overwritten by regeneration once those changes are in.

for more information, see https://pre-commit.ci

cloud/deployment/deployment.go

dimberman · 2025-06-18T00:24:06Z

cloud/deployment/deployment.go

 			}
 			switch dedicatedDeploymentRequest.Executor {
-			case astroplatformcore.UpdateDedicatedDeploymentRequestExecutorCELERY:
+			case astroplatformcore.UpdateDedicatedDeploymentRequestExecutorCELERY, astroplatformcore.UpdateDedicatedDeploymentRequestExecutorASTRO:


I worry a bit about all these cases of "if celery or astro executor" because there might be a point where we diverge somewhere and then the logic will get confusing (e.g. we need some special config just for astro). WDYT about us just making astro executor a separate case from the get-go? Slightly more code but cleaner conceptually.

🤷 I hear what you are saying, but my preference is to copy/paste logic until it's actually different.

I'll make some extra little helpers to re-use in both case statements to future proof w/o copy paste. thanks for this note.

Awesome, SGTM

cmd/airflow_test.go

neel-astro

Left some minor comments to work out, but functionally looks fine 👍

cloud/deployment/deployment.go

cloud/deployment/deployment_test.go

cmd/cloud/deployment.go

neel-astro · 2025-06-18T12:44:05Z

cmd/cloud/deployment.go


-func isValidExecutor(executor string) bool {
-	return strings.EqualFold(executor, deployment.KubeExecutor) || strings.EqualFold(executor, deployment.CeleryExecutor) || executor == "" || strings.EqualFold(executor, deployment.CELERY) || strings.EqualFold(executor, deployment.KUBERNETES)
+func isValidExecutor(executor, runtimeVersion string) bool {


I think we have the exact same method declared twice in two separate packages and could be moved into a single place.

Yeah it was already that way, but I took this opportunity to consolidate them, thanks for the note.

cloud/deployment/deployment.go

feluelle

LGTM, just a comment about testing, and a nit.

feluelle · 2025-06-19T06:47:52Z

cloud/deployment/deployment.go

The changes to this file appear more complex than I would expect. Can you also test manually that the deployment create/update requests for all executors still work as expected, including changing the executor on update?

yeah I had to do some refactoring to get around the gocyclo linter. good shout. I'll test create / update requests for all executors manually and post screen shots before merging.

cmd/cloud/deployment.go

feluelle · 2025-06-23T08:35:17Z

cmd/cloud/deployment.go

 	// check if executor is valid
-	if !isValidExecutor(executor) {
-		return fmt.Errorf("%s is %w", executor, errInvalidExecutor)
+	fmt.Println(deploymentType)


Suggested change

fmt.Println(deploymentType)

jaketf mentioned this pull request May 22, 2025

update oapi-codegen v2 #1855

Merged

8 tasks

jaketf force-pushed the af3-deployment-create branch from aeb5076 to cc6e6a2 Compare May 22, 2025 17:47

jaketf changed the title ~~AF3 / Remote Execution support for deployment create~~ AF3 Astro Executor support for deployment create Jun 13, 2025

jaketf force-pushed the af3-deployment-create branch from 9ebac35 to 8e471a4 Compare June 13, 2025 22:16

jaketf marked this pull request as ready for review June 13, 2025 22:28

jaketf requested review from jeremybeard, kushalmalani, lzdanski and neel-astro as code owners June 13, 2025 22:28

jaketf requested review from feluelle June 13, 2025 22:28

feluelle reviewed Jun 16, 2025

View reviewed changes

jaketf commented Jun 17, 2025

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

jaketf commented Jun 17, 2025

View reviewed changes

go.mod Show resolved Hide resolved

jaketf and others added 16 commits June 17, 2025 15:17

add astro exec validation rules

054be29

add astro exec validation rules

6b55b1b

[pre-commit.ci] auto fixes from pre-commit.com hooks

ae9f0c9

for more information, see https://pre-commit.ci

add remote execution to request

98b7aed

[revertme] outline changes to platform API

82a2798

this commit is just to outline explicitly the changes we need to make in the astro core platform API. It can be overwritten by regeneration once those changes are in.

wip fixing tests

cf0d140

wip

843dce8

[pre-commit.ci] auto fixes from pre-commit.com hooks

6d3fb42

for more information, see https://pre-commit.ci

remove remote execution from this PR

e60b220

regen

726f351

fixup with new api changes

0333e82

make mock

935e704

fixup tests

9c3db20

fixup assertion

2f02407

remove unit test for blocking af3 deployment creation

aa60d03

remove remote exec related args

e60334b

revert pre-commit changes

793fda2

jaketf force-pushed the af3-deployment-create branch from f0c8617 to 793fda2 Compare June 17, 2025 22:17

dimberman reviewed Jun 18, 2025

View reviewed changes

neel-astro reviewed Jun 18, 2025

View reviewed changes

jaketf commented Jun 18, 2025

View reviewed changes

cloud/deployment/deployment.go Outdated Show resolved Hide resolved

jaketf and others added 8 commits June 18, 2025 10:15

add issue to todo

474e402

fillWorkerQueues

35236ad

fixup worker queue filling

a43cf33

move more logic into update workers helper

019c87d

remove redundant logic

53715d9

Update cloud/deployment/deployment.go

8b34337

revert test quay change

f9b4cfa

fixup lint

3f73f00

jaketf requested review from dimberman and neel-astro June 18, 2025 19:27

dimberman approved these changes Jun 19, 2025

View reviewed changes

feluelle reviewed Jun 19, 2025

View reviewed changes

address review feedback

f017808

feluelle approved these changes Jun 20, 2025

View reviewed changes

jaketf added 4 commits June 20, 2025 08:33

fixup isAirflow3

940394b

fixup import

f4cb4f6

fix lint

edc3715

update fixes

5bf5bea

jaketf changed the title ~~AF3 Astro Executor support for deployment create~~ AF3 Astro Executor support for deployment create / update Jun 20, 2025

jaketf added 3 commits June 20, 2025 11:44

fixes to tests and update

5a55d1a

fixup lint

62d2ef2

fix update test

82379b6

feluelle reviewed Jun 23, 2025

View reviewed changes

jaketf merged commit 3e20e0a into main Jun 23, 2025
6 checks passed

jaketf deleted the af3-deployment-create branch June 23, 2025 21:16

Comments

Conversation

jaketf commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

🎟 Issue(s)

🧪 Functional Testing

📸 Screenshots

📋 Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neel-astro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feluelle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jaketf commented May 21, 2025 •

edited

Loading