Skip to content

Add executor flag to create deployment command#252

Merged
schnie merged 6 commits intomasterfrom
add-executor-flag
Jul 23, 2019
Merged

Add executor flag to create deployment command#252
schnie merged 6 commits intomasterfrom
add-executor-flag

Conversation

@andriisoldatenko
Copy link
Contributor

@andriisoldatenko andriisoldatenko commented Jul 12, 2019

fix #248

@andriisoldatenko
Copy link
Contributor Author

astro deployment create --help

Usage:
  astro deployment create DEPLOYMENT [flags]

Aliases:
  create, cr

Examples:

# Create new deployment with Celery executor (default: celery without params).
astro deployment create new-deployment-name --executor=celery

# Create new deployment with Local executor.
astro deployment create new-deployment-name-local --executor=local

# Create new deployment with Kubernetes executor.
astro deployment create new-deployment-name-k8s --executor=k8s


Flags:
  -e, --executor string   Add executor parameter: local or celery
  -h, --help              help for create

Global Flags:
      --workspace-id string   workspace assigned to deployment

@andriisoldatenko andriisoldatenko marked this pull request as ready for review July 17, 2019 14:44
@andriisoldatenko
Copy link
Contributor Author

 NAME            DEPLOYMENT NAME                  ASTRO              DEPLOYMENT ID
 new-new-k8s     interstellar-spacecraft-9873     0.10.0-alpha.3     cjy7csoud00bg0b35i9nwdn1t

 Successfully created deployment with KubernetesExecutor executor. Deployment can be accessed at the following URLs

 Airflow Dashboard: https://interstellar-spacecraft-9873-airflow.datarouter.ai
 Flower Dashboard: https://interstellar-spacecraft-9873-flower.datarouter.ai

@andriisoldatenko andriisoldatenko changed the title [WIP] Add executor flag Add executor flag to create deployment command Jul 17, 2019
@andriisoldatenko andriisoldatenko self-assigned this Jul 17, 2019
@andriisoldatenko andriisoldatenko added this to the v0.10 milestone Jul 17, 2019
@andriisoldatenko
Copy link
Contributor Author

@paolaperaza pls check this message -> Successfully created deployment with KubernetesExecutor executor. should i improve it?

@paolaperaza
Copy link
Contributor

Thanks for this, @andriisoldatenko ! A few things I can think of:

  • Could you take out that second executor? So, Successfully created deployment with the KubernetesExecutor?
  • Will we forcibly add the executor type to Name? So, if I create a deployment and name it "DEV" and add the k8s executor flag, will the name of the deployment always be DEV-k8s?
  • The Flower URL is specific to CeleryExecutor, so if a user picks LocalExecutor or Kubernetes, only the Airflow dashboard should render in the output

executorValue = "CeleryExecutor"
case "kubernetes", "k8s":
executorValue = "KubernetesExecutor"
default:
Copy link
Member

Choose a reason for hiding this comment

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

@andriisoldatenko wondering if we should remove the default fallback here and not specify an executor if they don't specify one. This will let the API determine the default executor, so we won't have to change it in multiple places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schnie one note: i can't send without, only with config: "" or similar. Otherwise i need to create 2 GraphQL queries.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, if you send an empty string does it default to celery at the api layer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when i'm trying to send empty config, in default scenario I get response like:

mutation CreateDeployment(
		$label: String!
		$type: String = "airflow"
		$workspaceId: Uuid!
		$config: JSON!
	) {
		createDeployment(
			label: $label
			type: $type
			workspaceUuid: $workspaceId
            config: $config
		) {
			id
			type
			label
			releaseName
			version
      config
			createdAt
			updatedAt
		}
	}

Response:

{
  "data": {
    "createDeployment": {
      "id": "cjyea2qp6010f0b78sbjk4nsm",
      "type": "airflow",
      "label": "test124",
      "releaseName": "solar-axis-9034",
      "version": "0.10.0-alpha.4",
      "config": {},
      "createdAt": "2019-07-22T10:59:27.785Z",
      "updatedAt": "2019-07-22T10:59:27.785Z"
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or when i'm trying to send config like "executor": "", i see error from API:

{
  "errors": [
    {
      "message": "Cannot read property 'components' of undefined",
      "locations": [
        {
          "line": 7,
          "column": 3
        }
      ],
      "path": [
        "createDeployment"
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR"
      },
      "level": "ERROR",
      "timestamp": "2019-07-22T11:02:19"
    }
  ],
  "data": {
    "createDeployment": null
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it looks like an empty string won't work - it has to be undefined for it to default currently. https://github.com/astronomer/houston-api/blob/master/src/lib/deployments/config/index.js#L120
https://lodash.com/docs/4.17.14#get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"config": {} works, i'm using it for emulate default value.

@andriisoldatenko
Copy link
Contributor Author

@schnie @paolaperaza

Celery executor example:

astro deployment create new-deployment-name --executor=celery
 NAME                    DEPLOYMENT NAME              ASTRO              DEPLOYMENT ID
 new-deployment-name     energetic-astronomy-9873     0.10.0-alpha.4     cjye9cn7600ww0b78hb772fet

 Successfully created deployment with Celery executor. Deployment can be accessed at the following URLs

 Airflow Dashboard: https://energetic-astronomy-9873-airflow.datarouter.ai
 Flower Dashboard: https://energetic-astronomy-9873-flower.datarouter.ai

Local executor example:

deployment create new-deployment-name-local --executor=local
 NAME                          DEPLOYMENT NAME               ASTRO              DEPLOYMENT ID
 new-deployment-name-local     advanced-singularity-1558     0.10.0-alpha.4     cjye9ksxl00y80b78ncylp00c

 Successfully created deployment with Local executor. Deployment can be accessed at the following URLs

 Airflow Dashboard: https://advanced-singularity-1558-airflow.datarouter.ai

@andriisoldatenko
Copy link
Contributor Author

andriisoldatenko commented Jul 22, 2019

Will we forcibly add the executor type to Name? So, if I create a deployment and name it "DEV" and add the k8s executor flag, will the name of the deployment always be DEV-k8s?

@schnie i need you approval about this feature please. ^

@schnie
Copy link
Member

schnie commented Jul 22, 2019

@andriisoldatenko I don't think we need to append the executor to the name.

@andriisoldatenko
Copy link
Contributor Author

@schnie I think i'm done, please take another look.

@schnie schnie merged commit 8980980 into master Jul 23, 2019
@schnie schnie deleted the add-executor-flag branch July 23, 2019 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Ability to choose Executor when running astro deployment create <deployment name> via the CLI

3 participants