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

[AIRFLOW-624] [AIRFLOW-784] [AIRFLOW-785] [AIRFLOW-777] Misc. fixes #2010

Merged
merged 4 commits into from Jan 22, 2017

Conversation

gsakkis
Copy link
Contributor

@gsakkis gsakkis commented Jan 22, 2017

@codecov-io
Copy link

codecov-io commented Jan 22, 2017

Current coverage is 66.88% (diff: 50.00%)

Merging #2010 into master will decrease coverage by 0.02%

@@             master      #2010   diff @@
==========================================
  Files           138        138          
  Lines         10531      10526     -5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           7046       7040     -6   
- Misses         3485       3486     +1   
  Partials          0          0          

Powered by Codecov. Last update 927f30c...2d2d627

@@ -205,7 +205,7 @@ def do_setup():
'flask-login==0.2.11',
'flask-swagger==0.2.13',
'flask-wtf==0.12',
'funcsigs>=1.0.2, <1.1',
Copy link
Contributor

Choose a reason for hiding this comment

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

You are setting it lower than we used before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm this ties it to the older version which would downgrade it at some other places. I'm not sure why we were dependent on >= 1.0.2, so I am not too happy by 'just' downgrading. We could also require a higher version of setup tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ultimately an issue with funcsigs, I'll open an issue for it.

We could also require a higher version of setup tools.

Afaict the setuptools requirement was what introduced the funcsigs bug for me; doing the same in Airflow would spread the issue more.

break

if not dag_is_running:
if not any(dag_run.state == State.RUNNING for dag_run in dag_runs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

@bolkedebruin
Copy link
Contributor

Nice fixes! Small nits / questions. Can you make sure to adhere to the "title" guidelines (mostly your cgroups commit messages is too long).

@gsakkis gsakkis changed the title [AIRFLOW-624] [AIRFLOW-784] [AIRFLOW-785] [AIRFLOW-777] [AIRFLOW-787] Misc. fixes [AIRFLOW-624] [AIRFLOW-784] [AIRFLOW-785] [AIRFLOW-777] Misc. fixes Jan 22, 2017
@bolkedebruin
Copy link
Contributor

I think you are missing a new line between the title of AIRFLOW0785 and its body. Github shows "..." which means it still exceeds to maximum amount of columns.

@bolkedebruin
Copy link
Contributor

Would be nice btw to have a test for AIRFLOW-777 (i.e. covering the function)

@gsakkis
Copy link
Contributor Author

gsakkis commented Jan 22, 2017

There is a new line, I think it's because above the 50 characters limit (59 to be precise). Feel free to suggest an alternative shorter subject but IMHO I'd rather pick a descriptive title than chop it off to to whatever arbitrarily small limit a particular git host uses for display.

@asfgit asfgit merged commit 2d2d627 into apache:master Jan 22, 2017
asfgit pushed a commit that referenced this pull request Jan 22, 2017
@gsakkis gsakkis deleted the fixes branch January 23, 2017 10:13
alekstorm pushed a commit to alekstorm/incubator-airflow that referenced this pull request Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants