Skip to content
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

Merged
merged 29 commits into from
Mar 11, 2022
Merged

Upgrade airflow #4940

merged 29 commits into from
Mar 11, 2022

Conversation

greenape
Copy link
Member

@greenape greenape commented Mar 7, 2022

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:

  • Existing airflow DBs should be backed up before starting, or any upgrade cannot be rolled back
  • The way in which airflow should be run has changed (previously all in one container, this is still possible but requires extra steps and should be avoided)

@greenape greenape added FlowETL Needs Review PR that is ready for review by a human labels Mar 7, 2022
@greenape greenape requested review from Thingus and jc-harrison March 7, 2022 11:10
@cypress
Copy link

cypress bot commented Mar 7, 2022



Test summary

43 0 0 0Flakiness 0


Run details

Project FlowAuth
Status Passed
Commit a59530e
Started Mar 11, 2022 9:33 AM
Ended Mar 11, 2022 9:42 AM
Duration 08:57 💡
OS Linux Debian - 10.5
Browser Electron 94

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"
Copy link
Contributor

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
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #4940 (a59530e) into master (d6b6357) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           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     
Impacted Files Coverage Δ
flowapi/flowapi/config.py 86.66% <ø> (ø)
flowetl/flowetl/setup.py 0.00% <ø> (ø)
flowetl/flowetl/flowetl/mixins/fixed_sql_mixin.py 100.00% <100.00%> (ø)
...wetl/flowetl/mixins/fixed_sql_with_params_mixin.py 100.00% <100.00%> (ø)
...owetl/flowetl/flowetl/mixins/wrapping_sql_mixin.py 100.00% <100.00%> (ø)
...wetl/flowetl/flowetl/operators/analyze_operator.py 100.00% <100.00%> (ø)
...operators/create_foreign_staging_table_operator.py 100.00% <100.00%> (ø)
...lowetl/flowetl/flowetl/sensors/file_flux_sensor.py 100.00% <100.00%> (ø)
flowetl/flowetl/flowetl/util.py 100.00% <100.00%> (+1.07%) ⬆️

📣 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}"
Copy link
Contributor

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?

Thingus
Thingus previously approved these changes Mar 7, 2022
Copy link
Contributor

@Thingus Thingus left a 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:
Copy link
Contributor

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?

@Thingus Thingus self-requested a review March 7, 2022 16:59
@Thingus Thingus dismissed their stale review March 7, 2022 17:00

Spotted a bare exception

@Thingus Thingus mentioned this pull request Mar 10, 2022
8 tasks
flowetl/flowetl/flowetl/util.py Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Member

@jc-harrison jc-harrison left a comment

Choose a reason for hiding this comment

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

👍

@Thingus Thingus added the ready-to-merge Label indicating a PR is OK to automerge label Mar 11, 2022
@mergify mergify bot merged commit 6ab2bd9 into master Mar 11, 2022
@mergify mergify bot deleted the bump-airflow branch March 11, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FlowETL Needs Review PR that is ready for review by a human ready-to-merge Label indicating a PR is OK to automerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants