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-4757] Selectively disable missing docstrings for tests #5400

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jun 10, 2019

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"
    • https://issues.apache.org/jira/browse/AIRFLOW-4757
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
    • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

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

Tests

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

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.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

  • Passes flake8

@potiuk potiuk requested a review from BasPH June 10, 2019 09:45
@potiuk potiuk force-pushed the missing-docstrings-not-required-for-tests branch from d9489d8 to 9efca0a Compare June 10, 2019 09:47
scripts/ci/ci_pylint.sh Outdated Show resolved Hide resolved
scripts/ci/ci_pylint.sh Outdated Show resolved Hide resolved
@potiuk potiuk force-pushed the missing-docstrings-not-required-for-tests branch from 9efca0a to 5646ab1 Compare June 10, 2019 10:38
@potiuk
Copy link
Member Author

potiuk commented Jun 10, 2019

@BasPH @ashb - I also added a few minor changes to make the bash script more "resilient" - now you can run it from any directory and it will work fine, I also added the usual -uo pipefail (I usually add set -euo pipefail for all bash scripts). I also disabled -x by default and replaced it with more informative messages (still left commented set -x so that you can uncomment it if needed)

@potiuk potiuk force-pushed the missing-docstrings-not-required-for-tests branch from 5646ab1 to de335e2 Compare June 10, 2019 10:49
# Uncomment to see the commands executed
# set -x

MY_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
Copy link
Member

Choose a reason for hiding this comment

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

I thought BASH_SOURCE was only for when you did source myscript not ./myscript - am I mis-remembering?

(BTW git has git rev-parse --show-toplevel which will print the abs-path to the root of the repo.)

Copy link
Member Author

@potiuk potiuk Jun 10, 2019

Choose a reason for hiding this comment

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

Not only - BASH_SOURCE array contains all bash scripts that were used. I always use this construct in all the bash scripts I run - this way I get them independent from where they are run.

See here for example: https://stackoverflow.com/questions/59895/get-the-source-directory-of-a-bash-script-from-within-the-script-itself

The problem with git is that not everywhere you have .git directory. For example we've learned that Dockerhub does not have the whole .git repo while building image. It's very similar in case of cloud build - there is no guarantee you have .git repo around. Of course in our case we know we have the repo in Travis, but not relying on it if we can is a better strategy IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to backup my statement on cloud build: GoogleCloudPlatform/cloud-builders#236


find . -name "*.py" \
-not -path "./.eggs/*" \
-not -path "./airflow/www/node_modules/*" \
-not -path "./airflow/_vendor/*" \
-not -path "./build/*" \
-not -path "./tests/*" \
Copy link
Member

Choose a reason for hiding this comment

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

git ls-files airflow/ | grep '.py' ?

Copy link
Member Author

@potiuk potiuk Jun 10, 2019

Choose a reason for hiding this comment

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

I like this, but again I have concerns that in some cases we might not have .git around. Do you think it is worth it to rely on .git dir @ashb ?

Copy link
Member

Choose a reason for hiding this comment

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

I can't see a case we'd want to run Pylint without a checkout as it's very much a CI-only thing. (I'm guessing we will volume mount in a checkout under the docker-based CI you will be working on?)

Copy link
Member Author

@potiuk potiuk Jun 10, 2019

Choose a reason for hiding this comment

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

Here is the case explained: GoogleCloudPlatform/cloud-builders#236 - the more "modern" CIs (such as cloud buid) work exclusively as Docker-first and usually they will ignore .git repo because otherwise it has to be passed as context to within Docker. That might slow-down the process a lot for big repositories as context is often sent to remote Docker server.

I am not saying we are going to do this, but I think building in a dependency on .git might not be future-proof as we know some CIs are already going in this direction.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is exactly what happens in Dockerhub, so this is not an isolated cloud-build case only.

Copy link
Member

@mik-laj mik-laj Jun 11, 2019

Choose a reason for hiding this comment

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

I use airflow-breeze to run Airflow, but I do small modification in my environment. I use a git worktree to eliminate the need to have multiple copies of the same repository in multiple workspace. In this case, the git does not work properly. Not relying on git is a very good idea.

scripts/ci/ci_pylint.sh Outdated Show resolved Hide resolved
@potiuk potiuk force-pushed the missing-docstrings-not-required-for-tests branch from de335e2 to 1905c9e Compare June 10, 2019 11:49
@potiuk
Copy link
Member Author

potiuk commented Jun 10, 2019

@ashb -> are you ok (at least for now) with not adding git to the picture? I'd love to merge that one. That can make developer's live a bit easier ;).

@ashb
Copy link
Member

ashb commented Jun 10, 2019

Oh yeah more than happy with this as it is.

@potiuk potiuk merged commit f710a0d into apache:master Jun 10, 2019
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
@potiuk potiuk deleted the missing-docstrings-not-required-for-tests branch September 19, 2019 22:46
@potiuk potiuk restored the missing-docstrings-not-required-for-tests branch September 19, 2019 22:46
@potiuk potiuk deleted the missing-docstrings-not-required-for-tests branch September 19, 2019 22:46
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.

4 participants