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

Flowetl refactoring & bug fixes #1380

Merged
merged 19 commits into from
Oct 9, 2019
Merged

Flowetl refactoring & bug fixes #1380

merged 19 commits into from
Oct 9, 2019

Conversation

maxalbert
Copy link
Contributor

Closes #1375, closes #1376, closes #1377, closes #1378, closes #1379.

I have:

  • Formatted any Python files with black
  • Brought the branch up to date with master
  • Added any relevant Github labels
  • Added tests for any new additions
  • Added or updated any relevant documentation
  • Added an Architectural Decision Record (ADR), if appropriate
  • Added an MPLv2 License Header if appropriate
  • Updated the Changelog

Description

  • The default_args section has been removed from the FlowETL config.yml. Similarly, we don't pass in a default_args dict to construct_etl_dag() any more - instead, the values for owner and start_date are now hard-coded inside that function because they don't need to change.

  • We now create DAGs only for those CDR types that are present in config.yml.

  • The config is now always validated and default values filled in when reading the config.yml file.

  • The concurrency setting from config.yml is now passed on to the DAG during creation, using the max_active_runs_per_dag parameter. Note that Airflow has a plethora of concurrency/parallelism settings, which can be a bit confusing (see discussions here, here, here and here, among other places), but max_active_runs_per_dag seems to be the right setting for this purpose.

  • The FlowETL deployment example has been updated so that it works when simply following the instructions.

@maxalbert maxalbert added bug Something isn't working refactoring FlowETL labels Oct 9, 2019
@cypress
Copy link

cypress bot commented Oct 9, 2019



Test summary

54 0 0 0


Run details

Project FlowAuth
Status Passed
Commit 81106f3
Started Oct 9, 2019 9:57 AM
Ended Oct 9, 2019 10:00 AM
Duration 02:40 💡
OS Linux Debian - 8.11
Browser Electron 61

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

@codecov
Copy link

codecov bot commented Oct 9, 2019

Codecov Report

Merging #1380 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1380      +/-   ##
==========================================
+ Coverage   95.01%   95.02%   +<.01%     
==========================================
  Files         157      157              
  Lines        7548     7551       +3     
  Branches      704      704              
==========================================
+ Hits         7172     7175       +3     
  Misses        268      268              
  Partials      108      108
Flag Coverage Δ
#flowapi_unit_tests 82.36% <ø> (ø) ⬆️
#flowauth_unit_tests 93.65% <ø> (ø) ⬆️
#flowclient_unit_tests 78.78% <ø> (ø) ⬆️
#flowetl_unit_tests 97.35% <100%> (+0.03%) ⬆️
#flowkit_jwt_generator_unit_tests 100% <ø> (ø) ⬆️
#flowmachine_unit_tests 90.86% <ø> (ø) ⬆️
#integration_tests 68.43% <ø> (ø) ⬆️
Impacted Files Coverage Δ
flowetl/etl/etl/etl_utils.py 100% <100%> (ø) ⬆️
flowetl/etl/etl/config_parser.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update faada91...81106f3. Read the comment docs.

@maxalbert maxalbert added the ready-to-merge Label indicating a PR is OK to automerge label Oct 9, 2019
Copy link
Member

@greenape greenape left a comment

Choose a reason for hiding this comment

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

👍

@mergify mergify bot merged commit 587a79f into master Oct 9, 2019
@mergify mergify bot deleted the flowetl-refactoring branch October 9, 2019 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment