Adds description field in variable (#12413) #15194

itroulli · 2021-04-04T21:24:59Z

This is my first PR. I tried to follow the CONTRIBUTING guide step by step. It passed all the pre-commit tests.

My PR adds a description field for the Airflow variables that can only be set and seen trhough the UI. Also, I would prefer to have created a textarea class instead of an input class but I wasn't able to find a way to do it. Any advice on that will be appreciated.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

boring-cyborg · 2021-04-04T21:25:01Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

kurtqq · 2021-04-05T20:47:59Z

thanks @itroulli !

itroulli · 2021-04-06T13:25:39Z

Thank you @kurtqq for the guidance! If you have any suggestion on how to make the field a textarea one, let me know!

xinbinhuang

LGTM

github-actions · 2021-04-09T05:26:57Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

airflow/www/views.py

airflow/models/variable.py

xinbinhuang · 2021-04-09T19:43:24Z

@itroulli I help rebase the branch because any changes in the migration folder require to rebase on the latest master before merging. Once CI passed, I will merge it

itroulli · 2021-04-09T20:22:59Z

@itroulli I help rebase the branch because any changes in the migration folder require to rebase on the latest master before merging. Once CI passed, I will merge it

Thanks a lot for the help! I appreciate it!

xinbinhuang · 2021-04-09T22:09:51Z

airflow/models/variable.py

@@ -47,12 +47,14 @@ class Variable(Base, LoggingMixin):
    id = Column(Integer, primary_key=True)
    key = Column(String(ID_LEN), unique=True)
    _val = Column('val', Text)
+    description = Column(Text(5000))


Suggested change

description = Column(Text(5000))

description = Column(Text)

@itroulli sorry for the confusion. Postgres doesn't allow modifier on text field. Can you also update this and the alembic migration file? (you would need to git pull the branch first) Let me know if you have any questions :)

I think I'd need some help with updating the revision keys in the migration file. I believe this is why some checks are failing. With Python 3.6, MySQL 5.7 right now I get this error:
alembic.util.exc.CommandError: Can't locate revision identified by '458c2838cbca'

Hmm, this is because there are other migration changes merged to the master before you, which happens sometimes given the number of PRs we have. We used to have problems with multiple diverge alembic heads, and we mitigate it by making sure that any PR affecting the migration folder will need to rebase on the latest master. So, to fix it, I normally delete the revision file and regenerate it after rebase to master

This is the most recent change #15203

Seems that everything is passing now! :)

github-actions · 2021-04-11T21:01:02Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

github-actions · 2021-04-11T21:01:30Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

github-actions · 2021-04-11T21:01:38Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

Co-authored-by: itroulli

…h Postgres

boring-cyborg · 2021-04-12T15:33:28Z

Awesome work, congrats on your first merged pull request!

xinbinhuang · 2021-04-12T15:34:00Z

@itroulli Thank you so much for the PR!

kaxil · 2021-04-12T17:03:55Z

airflow/models/variable.py

+    def __init__(self, key=None, val=None, description=None):
        super().__init__()
        self.key = key
        self.val = val
+        self.description = description


Can we add some tests for this feature please

@kaxil Since this is a feature only viewable/editable from the UI, what tests would you suggest? Just including the description field in this line would suffice?
Thanks in advance!

Probably just a separate test in that file, the simply tests that you can set description to a variable, which is peristed to db and when you do Variable.get you get the same description field

@kaxil The thing is that following @kurtqq 's suggestion in the issue, I didn't modify the Variable.get method so it won't return the description.

I may be missing something though, let me know what do you think.

That is debatable :) -- but anyways for now you could just do session.query(Variable.key, Variable.description).first() and just verify that

Thanks a lot for the tips! I rebased, implemented it and pushed in my branch but I guess a new PR is needed, right?

I've created #15400. Thanks again!

Related: #15194 In continuation of PR #15194, I've added a test for the description field of the variable.

itroulli requested review from ashb, kaxil, ryanahamilton, turbaszek and XD-DENG as code owners April 4, 2021 21:24

boring-cyborg bot added the area:webserver Webserver related Issues label Apr 4, 2021

itroulli force-pushed the issue_12413 branch from a3a998f to 2a8f66e Compare April 4, 2021 21:36

xinbinhuang approved these changes Apr 9, 2021

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Apr 9, 2021

xinbinhuang reviewed Apr 9, 2021

View reviewed changes

airflow/www/views.py Show resolved Hide resolved

xinbinhuang self-requested a review April 9, 2021 06:43

xinbinhuang reviewed Apr 9, 2021

View reviewed changes

airflow/models/variable.py Outdated Show resolved Hide resolved

xinbinhuang self-requested a review April 9, 2021 06:55

xinbinhuang approved these changes Apr 9, 2021

View reviewed changes

xinbinhuang force-pushed the issue_12413 branch 2 times, most recently from bf7fa19 to d97e70d Compare April 9, 2021 19:33

xinbinhuang reviewed Apr 9, 2021

View reviewed changes

xinbinhuang self-requested a review April 9, 2021 22:10

itroulli force-pushed the issue_12413 branch from 5907498 to 3f04747 Compare April 11, 2021 13:43

Ilias Troullinos and others added 2 commits April 12, 2021 00:15

add description field in variable (apache#12413)

7e7040b

Change description file to Text

5208b9d

Co-authored-by: itroulli

xinbinhuang and others added 3 commits April 12, 2021 00:15

Rebase migration on master

763795e

remove length parameter from description column for compatibility wit…

91a5d95

…h Postgres

update migration revision

1d189d9

itroulli force-pushed the issue_12413 branch from 3f04747 to 1d189d9 Compare April 11, 2021 22:20

delete past migration revision

aa1b80e

xinbinhuang merged commit 925ef28 into apache:master Apr 12, 2021

ashb added this to the Airflow 2.1 milestone Apr 12, 2021

kaxil reviewed Apr 12, 2021

View reviewed changes

itroulli mentioned this pull request Apr 16, 2021

Adds a test for the description field in variable #15400

Merged

kaxil pushed a commit that referenced this pull request Apr 16, 2021

Adds a test for the description field in variable (#15400)

e7c642b

Related: #15194 In continuation of PR #15194, I've added a test for the description field of the variable.

eladkal mentioned this pull request Oct 4, 2023

Expose description columns of Pools in the UI #34751

Closed

1 task

Adds description field in variable (#12413) #15194

Adds description field in variable (#12413) #15194

Uh oh!

Conversation

itroulli commented Apr 4, 2021

Uh oh!

boring-cyborg bot commented Apr 4, 2021

Uh oh!

kurtqq commented Apr 5, 2021

Uh oh!

itroulli commented Apr 6, 2021

Uh oh!

xinbinhuang left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 9, 2021

Uh oh!

Uh oh!

Uh oh!

xinbinhuang commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itroulli commented Apr 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xinbinhuang Apr 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 11, 2021

Uh oh!

github-actions bot commented Apr 11, 2021

Uh oh!

github-actions bot commented Apr 11, 2021

Uh oh!

boring-cyborg bot commented Apr 12, 2021

Uh oh!

xinbinhuang commented Apr 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xinbinhuang commented Apr 9, 2021 •

edited

Loading

xinbinhuang Apr 11, 2021 •

edited

Loading