-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade airflow #4940
Upgrade airflow #4940
Conversation
Need to init before it'll be ready
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
@@ -121,7 +121,7 @@ def test_module_path_added_to_dag_template_locations(): | |||
dag = DAG("DUMMY_DAG", start_date=datetime.now()) | |||
get_qa_checks(dag=dag) | |||
assert ( | |||
str(Path(__file__).parent.parent.parent / "flowetl" / "flowetl" / "qa_checks") | |||
Path(__file__).parent.parent.parent / "flowetl" / "flowetl" / "qa_checks" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the extra layer of 'flowetl' in the tree, or can that be flattened out?
Codecov Report
@@ Coverage Diff @@
## master #4940 +/- ##
=======================================
Coverage 93.91% 93.92%
=======================================
Files 276 276
Lines 10767 10762 -5
Branches 1306 1304 -2
=======================================
- Hits 10112 10108 -4
Misses 522 522
+ Partials 133 132 -1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
# Make sure we have a db connection specified | ||
|
||
: "${AIRFLOW__CORE__SQL_ALCHEMY_CONN:?AIRFLOW__CORE__SQL_ALCHEMY_CONN env var or secret must be set.}" | ||
|
||
# Defaults and back-compat | ||
: "${AIRFLOW_HOME:="/opt/airflow"}" | ||
: "${AIRFLOW__CORE__FERNET_KEY:=${FERNET_KEY:=$(python -c "from cryptography.fernet import Fernet; FERNET_KEY = Fernet.generate_key().decode(); print(FERNET_KEY)")}}" | ||
: "${AIRFLOW__CORE__EXECUTOR:=${EXECUTOR:-Sequential}Executor}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Airflow recommends local executor for non-dev purposes - worth setting that as the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - I'd say it's worth editing the compose files so we have flowetl_scheduler, flowetl_webserver (both just flowetl with the appropriate entry point), but that's a related PR I think, Just needs codecov to be happy.
# See if the sql to be wrapped lives in a file | ||
jinja_env = self.dag.get_template_env() | ||
sql = jinja_env.loader.get_source(jinja_env, self.sql)[0] | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need an explicit exception here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This updates Airflow to 2.2.3.
It dispenses with most of our startup script for FlowETL, and of necessity will be a breaking change release because airflow updates are not reversible.
Need to add some update notes for this, because: