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-2853] Add official committers to README #3699

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@andscoop
Contributor

andscoop commented Aug 5, 2018

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"

Description

  • Here are some details about my PR, including screenshots of any UI changes:

This PR brings a list of maintainers into the code repo so that they can easily be found and live with the repo itself.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Docs

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 (not including Jira issue reference)
    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"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff
README.md Outdated
1. [Patrick Leo Tardif](https://github.com/patrickleotardif)
1. [Siddharth Anand](https://github.com/r39132)
1. Steven Yvinec-Kruyk
1. [Sumit Maheshwari](https://github.com/msumit)

This comment has been minimized.

@ashb

ashb Aug 5, 2018

Member

This list is slightly out of date - you are missing some people, and Alex Guziel isn't a commiter? - the authoritative list is http://people.apache.org/committers-by-project.html#airflow

This comment has been minimized.

@andscoop

andscoop Aug 5, 2018

Contributor

@ashb Perfect, I was looking for this and was unable to find it. I was going off of https://incubator.apache.org/projects/airflow.html . Will update

This comment has been minimized.

@andscoop

andscoop Aug 5, 2018

Contributor

@ashb I have updated according to that list, just a note, I left Alex on as he is on the list you linked to.

This comment has been minimized.

@ashb

ashb Aug 5, 2018

Member

Oh so he is - it's just sorted by user id :)

@codecov-io

This comment has been minimized.

codecov-io commented Aug 5, 2018

Codecov Report

Merging #3699 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3699      +/-   ##
==========================================
- Coverage   77.56%   77.55%   -0.01%     
==========================================
  Files         204      204              
  Lines       15766    15766              
==========================================
- Hits        12229    12228       -1     
- Misses       3537     3538       +1
Impacted Files Coverage Δ
airflow/models.py 88.54% <0%> (-0.05%) ⬇️

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 3157287...b583f21. Read the comment docs.

@ashb

ashb approved these changes Aug 5, 2018

@r39132

This comment has been minimized.

Contributor

r39132 commented Aug 5, 2018

@andscoop In your Jira, you referenced creating a CODEOWNERS files. Why is that not the way to proceed?

@andscoop

This comment has been minimized.

Contributor

andscoop commented Aug 5, 2018

@r39132 I did not originally research thoroughly.

It would require us to set owners to various sections of the codebase so that those owners are prompted for automatic review when a PR for that section is submitted. CODEOWNERS may be the direction the project wants to go eventually - but I don't believe it (the project) currently goes into the level of detail of accepting committers for specific sections of the codebase. While some committers may be more inclined to review PRs from a specific section, from the outside looking in, it appears that most committers/maintainers float across various sections of the codebase.

@r39132

This comment has been minimized.

Contributor

r39132 commented Aug 5, 2018

Thx @andscoop. I agree we are not there yet.

Ok, so I sent a note to the dev list that we have 3 places currently where the committer list is maintained : https://lists.apache.org/thread.html/15cf9c3f0d45a08f988fe9f3ff0fa6092ef8a8655e6087177fbc4a51@%3Cdev.airflow.apache.org%3E

Merging this PR would require yet another list to be updated. This is why I originally linked via the cwiki. If you look at the cwiki right now, I did add all 3 places where committer lists are maintained. Does this satisfy your need?

@r39132

This comment has been minimized.

Contributor

r39132 commented Aug 5, 2018

I do like what you have written though and would love you to alter the cwiki page. I was trying to add you an admin to the cwiki, but could not find you for some reason. I tried to add andscoop and Andy Cooper. if you go by some other name, do let me know.

@andscoop

This comment has been minimized.

Contributor

andscoop commented Aug 5, 2018

@r39132 please try again - just officially signed up for the cwiki which is apparently different than the jira signup.

I understand the concern with maintaining this is 3 different places. Main reason for the PR is IMO that docs which don't live with the code get left behind. I will gladly update the wiki if we choose not to move forward with the PR.

@r39132

This comment has been minimized.

Contributor

r39132 commented Aug 5, 2018

@andscoop I granted you access... though I see in your PR, that you do still link to the cwiki. You may not have much to update in the wiki itself.

I do like the "Who maintains Airflow" section that you added. I would be open to merging this PR if you didn't have the explicit list of committers listed .. we have enough PRs to merge as it is and there a quite a few steps involved in promoting a contributor to committer status -- I'd be happy not to add yet another step to it.

That said, you could add a link to https://github.com/orgs/apache/teams/airflow-committers/members on the read me so people could see the list of committers in a single hop.

Would this work for you?

@andscoop

This comment has been minimized.

Contributor

andscoop commented Aug 5, 2018

@r39132 Looks like a couple of these links need team access to github and access to whimsy server. So we can't put those in the file. The best definitive resource I have seen so far is the one @ashb linked to in review. Is this the link we should move forward with?

edit - removed list of contributed and updated with list of known working links

@r39132

This comment has been minimized.

Contributor

r39132 commented Aug 6, 2018

LGTM!

@r39132 r39132 closed this in dc64649 Aug 6, 2018

lxneng added a commit to lxneng/incubator-airflow that referenced this pull request Aug 10, 2018

[AIRFLOW-2853] Add official committers to README
Closes apache#3699 from andscoop/Add-core-commiters-to-
readme

dlebech added a commit to trustpilot/incubator-airflow that referenced this pull request Sep 11, 2018

[AIRFLOW-2853] Add official committers to README
Closes apache#3699 from andscoop/Add-core-commiters-to-
readme

dalupus added a commit to modmed/incubator-airflow that referenced this pull request Sep 19, 2018

[AIRFLOW-2853] Add official committers to README
Closes apache#3699 from andscoop/Add-core-commiters-to-
readme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment