Get airflow to work with official docker image #67
Conversation
dimberman
requested review from
ashb,
schnie and
sjmiller609
and removed request for
schnie
dimberman
force-pushed
the
airflow-2.0-compliance
branch
from
71610ee to
1d39160
Compare
dimberman
changed the title
|
@schnie I'm still waiting on apache/airflow#8219 before I can use the actual official image, but we should start the review process now :) |
| - "users" | ||
| - "create" | ||
| - "--role" |
There was a problem hiding this comment.
this commands will depend on the version (Master uses users create but 1.10.* uses create_user
There was a problem hiding this comment.
@kaxil unfortunately to merge this to master we'll need to make it 2.0 compliant, and THEN make it 1.10 complaint AGAIN when we cherry-pick to the 1-10-test branch.
There was a problem hiding this comment.
These args are fine on 1.10, it's just the new command names that are the issue.
I wonder if we should add the new commands to 1.10.11 -- it would make upgrading easier on people.
dimberman
force-pushed
the
airflow-2.0-compliance
branch
from
ad135db to
b62e0a9
Compare
To merge our chart into the airflow repo, we need the ability to run with an airflow 2.0 official image
dimberman
force-pushed
the
airflow-2.0-compliance
branch
from
b62e0a9 to
36943a9
Compare
There was a problem hiding this comment.
We need to make sure that all supported AC versions will work with this chart, which I have validated is the case for airflow chart version 0.12 (latest patch verison). Here is an issue that summarizes my concern https://github.com/astronomer/issues/issues/1132
| image: {{ template "airflow_image" . }} | ||
| imagePullPolicy: {{ .Values.images.airflow.pullPolicy }} | ||
| args: ["airflow-migration-spinner", "--timeout=60"] | ||
| args: ["db", "check-migrations", "--migration-wait-timeout", "60"] |
There was a problem hiding this comment.
The airflow-chart needs to support all astronomer certified images. Do these modifications to args work with all published AC versions?
There was a problem hiding this comment.
@sjmiller609 there isn't any non horrible way we can create a chart that both works for 1.10 and 2.0. Maybe we need to fork this repo?
There was a problem hiding this comment.
To merge into master it needs to work with master, then we can cherry-pick and fix it for the 1-10-test branch
There was a problem hiding this comment.
@schnie can you decide what to do for this one? The only item I wish to protect in this case is AC compatibility on the master and release branches of this repository.
There was a problem hiding this comment.
I'm cool with us forking "astronomer/oss-airflow-chart" and then donating that. Would that work?
There was a problem hiding this comment.
I'll let greg speak to what pattern he wants, I think a branch on this repo seems reasonable, the benefit as opposed to a fork being that CI will keep working without further modification to CI configuration.
my suggestion:
- branch on this repo (checkout -b from master), add branch protection rules in admin settings of this repository
- This PR into that branch
- when it is donation-ready, then copy it into greg's personal fork of airflow
- Commit to include all current authors of the airflow chart
- PR to apache/airflow
It seems that this function is already taken care of internally
|
Closing stale PR |
6 participants
This PR works, but we need to figure out how to modify the official airflow docker image to match the one provided here.