Skip to content

[AIRFLOW-2227] allow the ability to delete an airflow variable using the variable class #3142

Closed
nadav-baruryan-lmnd wants to merge 4 commits intoapache:masterfrom
nadav-baruryan-lmnd:AIRFLOW-2227
Closed

[AIRFLOW-2227] allow the ability to delete an airflow variable using the variable class #3142
nadav-baruryan-lmnd wants to merge 4 commits intoapache:masterfrom
nadav-baruryan-lmnd:AIRFLOW-2227

Conversation

@nadav-baruryan-lmnd
Copy link

@nadav-baruryan-lmnd nadav-baruryan-lmnd commented Mar 19, 2018

Make sure you have checked all steps below.

JIRA

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    In order to allow using migration (via alembic or customized scripts) one need the ability to delete variables (delete function).Another utility function was added to allow retrieval of all the current variables keys.t. This PR allow these abilities.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    Variable methods in models.py has no tests Link

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":

    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"
  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

@codecov-io
Copy link

codecov-io commented Mar 20, 2018

Codecov Report

Merging #3142 into master will decrease coverage by 3.55%.
The diff coverage is 31.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3142      +/-   ##
==========================================
- Coverage   76.47%   72.91%   -3.56%     
==========================================
  Files         203      180      -23     
  Lines       15012    12673    -2339     
==========================================
- Hits        11480     9241    -2239     
+ Misses       3532     3432     -100
Impacted Files Coverage Δ
airflow/models.py 86.86% <31.57%> (-0.04%) ⬇️
airflow/operators/sensors.py 0% <0%> (-100%) ⬇️
airflow/operators/redshift_to_s3_operator.py 0% <0%> (-95.46%) ⬇️
airflow/operators/s3_file_transform_operator.py 0% <0%> (-93.62%) ⬇️
airflow/sensors/named_hive_partition_sensor.py 0% <0%> (-89.19%) ⬇️
airflow/sensors/web_hdfs_sensor.py 0% <0%> (-50%) ⬇️
airflow/sensors/s3_prefix_sensor.py 0% <0%> (-41.18%) ⬇️
airflow/sensors/metastore_partition_sensor.py 0% <0%> (-40%) ⬇️
airflow/sensors/hive_partition_sensor.py 0% <0%> (-36.37%) ⬇️
airflow/sensors/s3_key_sensor.py 0% <0%> (-31.04%) ⬇️
... and 191 more

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 4ce2502...631c2a3. Read the comment docs.

@nadav-baruryan-lmnd nadav-baruryan-lmnd changed the title Airflow 2227 [AIRFLOW - 2227] allow the ability to delete an airflow variable using the variable class Mar 22, 2018
def delete(cls, key, session=None):
try:
session.query(cls).filter(cls.key == key).delete()
session.commit()
Copy link
Member

Choose a reason for hiding this comment

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

This seems suspect - why do we want to commit the session here - shouldn't the caller be in change of the session instead?

session.commit()
except Exception as e:
session.rollback()
logging.warning("Failed to delete key {}".format(key))
Copy link
Member

Choose a reason for hiding this comment

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

"variable", not key.


@classmethod
@provide_session
def get_keys(cls, session=None):
Copy link
Member

Choose a reason for hiding this comment

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

What's this method for?

@ashb ashb changed the title [AIRFLOW - 2227] allow the ability to delete an airflow variable using the variable class [AIRFLOW-2227] allow the ability to delete an airflow variable using the variable class Nov 1, 2018
@stale
Copy link

stale bot commented Dec 16, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 16, 2018
@stale stale bot closed this Dec 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants