github action tests #8222

dimberman · 2020-04-08T19:45:47Z

Move all k8s tests to github actions so we can be free from Travis!

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

resolves #8221

zhongjiajie · 2020-04-09T01:36:24Z

Looking forward on it.

dimberman · 2020-04-09T04:57:17Z

@zhongjiajie please don't judge my commit messages. I just have to launch this so many times :P

zhongjiajie · 2020-04-09T05:19:34Z

Yeah, I know that, I try in my repo days ago but failed and can not find solution, It's really take times and patient, but I know we can make it

dimberman · 2020-04-09T17:35:26Z

@potiuk @ashb @kaxil @turbaszek I think it would make sense to to just jump straight to using the helm chart rather than trying to retrofit the old k8s tests to work with github issues. I was running into all sorts of weird issues and frankly all of that templating needs to be thrown away (we're basically doing a jankier version of helm with those deployments.).

Once my PR is in to make our helm chart compliant with airflow 2.0 is it ok if we merge in before merging the helm chart? Or should I just maintain this branch until it is ready?

turbaszek

I love it! It's definitely much simpler :)

.github/workflows/ci-k8s.yml

turbaszek · 2020-04-09T19:27:55Z

.github/workflows/ci-k8s.yml

+on: [pull_request, push]
+
+jobs:
+  create-airflow-dockerfile:


Should we adjust this name?

kaxil · 2020-04-09T19:29:43Z

scripts/ci/ci_run_airflow_testing.sh

-    set +u
-    # shellcheck disable=SC2016
-    docker-compose --log-level INFO \
-      -f "${MY_DIR}/docker-compose/base.yml" \
-      -f "${MY_DIR}/docker-compose/backend-${BACKEND}.yml" \
-      "${INTEGRATIONS[@]}" \
-      "${DOCKER_COMPOSE_LOCAL[@]}" \
-         run airflow \
-           '/opt/airflow/scripts/ci/in_container/entrypoint_ci.sh "${@}"' \
-           /opt/airflow/scripts/ci/in_container/entrypoint_ci.sh "${@}"
-         # Note the command is there twice (!) because it is passed via bash -c
-         # and bash -c starts passing parameters from $0. TODO: fixme
-    set -u


This (else part) should still remain I think

How come? We're not running those travis tests anymore

I thought only the "if" part ran Kubernetes tests

cc @potiuk Can you verify if we need the else part or is it safe to remove, please :)

We definitely need the else. all tests are using it but the kubernetes ones.

turbaszek · 2020-04-09T19:31:48Z

@dimberman one question: do we run any tests? I checked one job and it seems all were skipped. It seems that we need integration flag and runtime.

.github/workflows/ci-k8s.yml

potiuk · 2020-04-10T15:04:08Z

Looking at it now :) @dimberman !

potiuk

It looks good for just the kubernetes tests but It needs to be fixed for all other (the else in scripts :)) .

It's ok to run it as it is now and once it works with Github Actions I will convert Kubernetes runtime into kubenetes "integration" in the similar way we run them with Breeze now. But it can be a next step after this one works.

However - as I understand it - the way it works now is not cool - we need to connect the locally built production image with the helm chart and use it rather than install helm chart which installs "some" version of Airflow.. Otherwise what we are testing is "some" airflow already released as image - but not Airflow built on top of the current sources. So we are not really testing the current sources - we are testing the "other" sources (already built and published before). That was already how it was working before I modified it last time (when @gerardo ) prepared it. The result of it was that when there were breaking changes in Kubernetes master, they were only discovered after the changes were merged to master and the image was rebuilt - and then the other PRs started to fail. This is not Cool and we need to fix it before merge. I am happy to help, but I think we need the helm chart to be part of this change rather than pulled from astronomer.

potiuk · 2020-04-10T15:05:42Z

.github/workflows/ci-k8s.yml

+         ./get_helm.sh
+      - name: create namespace
+        run: kubectl create namespace airflow
+      - name: pull astronomer helm chart


I guess we will switch to the airflow's helm chart eventually :) ?

Honestly the setup we have right now is just a super janky version of helm (using SED for templating). It's really unflexible and a LOT of code. figure this would make our tests closer to production.

potiuk · 2020-04-10T15:06:17Z

.github/workflows/ci-k8s.yml

+        run: kubectl create namespace airflow
+      - name: pull astronomer helm chart
+        run: |
+         helm repo add astronomer https://helm.astronomer.io


Should we create a new repo now? Happy to help with that.

@schnie thoughts?

potiuk · 2020-04-10T15:09:59Z

.github/workflows/ci-k8s.yml

+         kubectl get pods --namespace airflow
+         kubectl get svc --namespace airflow
+         kubectl port-forward --namespace airflow svc/airflow-webserver 8080:8080 &
+      - name: pip install airflow


We can pull and use CI image to run the tests on - I am happy to work on it. this way we won't have to install airflow locally and we should be able to repeat it easily using breeze. The problem which Breeze /CI image solves is to make sure that local environment does not have impact on running the tests. But it can be done as a follow-up PR easily.

potiuk · 2020-04-10T16:15:50Z

scripts/ci/ci_run_airflow_testing.sh

-    set +u
-    # shellcheck disable=SC2016
-    docker-compose --log-level INFO \
-      -f "${MY_DIR}/docker-compose/base.yml" \
-      -f "${MY_DIR}/docker-compose/backend-${BACKEND}.yml" \
-      "${INTEGRATIONS[@]}" \
-      "${DOCKER_COMPOSE_LOCAL[@]}" \
-         run airflow \
-           '/opt/airflow/scripts/ci/in_container/entrypoint_ci.sh "${@}"' \
-           /opt/airflow/scripts/ci/in_container/entrypoint_ci.sh "${@}"
-         # Note the command is there twice (!) because it is passed via bash -c
-         # and bash -c starts passing parameters from $0. TODO: fixme
-    set -u


We definitely need the else. all tests are using it but the kubernetes ones.

dimberman · 2020-04-10T16:27:48Z

@potiuk yes I have a PR I'm working on that will allow us to run the official airflow chart astronomer/airflow-chart#67. Right now I'm just getting it to run AT ALL before getting it to run correctly (eventually this build will have a step to build the docker image and run it, but I took that out because it took a lot of time between iterations)

ashb · 2020-04-20T16:30:50Z

@dimberman We can close this now?

zhongjiajie · 2020-04-21T09:56:48Z

I think we can.

boring-cyborg bot added the area:dev-tools label Apr 8, 2020

dimberman force-pushed the k8s-tests-github-actions branch from 13e8213 to b461712 Compare April 9, 2020 05:00

boring-cyborg bot added the k8s label Apr 9, 2020

dimberman requested review from potiuk, turbaszek, ashb and kaxil April 9, 2020 17:35

dimberman force-pushed the k8s-tests-github-actions branch from 7556852 to 48f170f Compare April 9, 2020 18:07

turbaszek approved these changes Apr 9, 2020

View reviewed changes

.github/workflows/ci-k8s.yml Outdated Show resolved Hide resolved

.github/workflows/ci-k8s.yml Outdated Show resolved Hide resolved

.github/workflows/ci-k8s.yml Outdated Show resolved Hide resolved

turbaszek reviewed Apr 9, 2020

View reviewed changes

.github/workflows/ci-k8s.yml Outdated

on: [pull_request, push]

jobs:

create-airflow-dockerfile:

Copy link

Member

turbaszek Apr 9, 2020

Choose a reason for hiding this comment

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

Should we adjust this name?

kaxil reviewed Apr 9, 2020

View reviewed changes

dimberman force-pushed the k8s-tests-github-actions branch 2 times, most recently from fe5095c to a7baf0b Compare April 9, 2020 20:37

turbaszek reviewed Apr 9, 2020

View reviewed changes

.github/workflows/ci-k8s.yml Outdated Show resolved Hide resolved

potiuk requested changes Apr 10, 2020

View reviewed changes

potiuk mentioned this pull request Apr 12, 2020

Kubernetes on host #8265

Merged

5 tasks

astro-sql-decorator added 2 commits April 14, 2020 10:38

Migration script

35a8d8f

github actions for k8s tests

47d9b9c

dimberman force-pushed the k8s-tests-github-actions branch from 34fc432 to 47d9b9c Compare April 14, 2020 19:12

boring-cyborg bot added the area:CLI label Apr 14, 2020

astro-sql-decorator added 2 commits April 14, 2020 12:20

Asfadsf

2947650

Asfadsf

33657aa

dimberman closed this May 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

github action tests #8222

github action tests #8222

dimberman commented Apr 8, 2020 •

edited

Loading

zhongjiajie commented Apr 9, 2020

dimberman commented Apr 9, 2020

zhongjiajie commented Apr 9, 2020

dimberman commented Apr 9, 2020

turbaszek left a comment

turbaszek Apr 9, 2020

kaxil Apr 9, 2020

dimberman Apr 9, 2020

kaxil Apr 9, 2020

potiuk Apr 10, 2020

turbaszek commented Apr 9, 2020 •

edited

Loading

potiuk commented Apr 10, 2020

potiuk left a comment

potiuk Apr 10, 2020

dimberman Apr 10, 2020

potiuk Apr 10, 2020

dimberman Apr 10, 2020

potiuk Apr 10, 2020

potiuk Apr 10, 2020

dimberman commented Apr 10, 2020

ashb commented Apr 20, 2020

zhongjiajie commented Apr 21, 2020

github action tests #8222

github action tests #8222

Conversation

dimberman commented Apr 8, 2020 • edited Loading

zhongjiajie commented Apr 9, 2020

dimberman commented Apr 9, 2020

zhongjiajie commented Apr 9, 2020

dimberman commented Apr 9, 2020

turbaszek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbaszek commented Apr 9, 2020 • edited Loading

potiuk commented Apr 10, 2020

potiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimberman commented Apr 10, 2020

ashb commented Apr 20, 2020

zhongjiajie commented Apr 21, 2020

dimberman commented Apr 8, 2020 •

edited

Loading

turbaszek commented Apr 9, 2020 •

edited

Loading