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

Update Contributing docs and add tox as test optional-dependency #4209

Merged
merged 6 commits into from
Jun 5, 2024

Conversation

hdub-tech
Copy link
Contributor

DISCLAIMER

Long time ansible-lint user, first time contributor, and also first time working
directly with a Python project. I'm trying to do things "right" but might have
missed the mark. Feedback welcome so I can get better!

Description

Problem: The contributing docs are a little lacking on how to get up and go and,
while they encourage running tox tests, tox is not included as a test dependency.

Solution: This PR

  1. Improves documentation aimed at first time Contributors
  2. Adds tox as an optional test dependency
  3. Adds some common venv paths to .gitignore
  4. Cleans up some output for tox list
  5. Cleans up some output in tox run -e hook

Testing

  • tox installed with pip install -e .[test]

    BEFORE:
    ansiblelint-install-tox-before

    AFTER:
    ansiblelint-install-tox-after
    ansiblelint-install-tox-after-2

  • git ignores .venv

    BEFORE:
    ansiblelint-ignore-venv-before

    AFTER:
    ansiblelint-ignore-venv-after

  • tox list no longer has a dangling and

    BEFORE:
    ansiblelint-tox-and-before

    AFTER:
    ansiblelint-tox-and-after

  • Add --initial-branch=main to test-hooks.sh to get rid of big warning.

    BEFORE:
    ansiblelint-tox-hook-before

    AFTER:
    ansiblelint-tox-hook-after

  • tox run -e lint,pkg,docs,py

    ansiblelint-final-tox-run

Outstanding questions / TODOs

If someone has the answers for these, I'm willing to update this PR to include it.

  1. If there was a test that should be updated / added for this PR, please point me to the desired
    location and I will do the needful.
  2. Should the DCO file be removed since the --signoff requirement was removed in Clean up the DCO & --signoff mentions from CONTRIBUTING.md #719?
  3. I didn't update the docs to mention the need to have npm installed for the tox run -e schemas.
    I see the workflows are using setup-node and a cache-dependency-path but I didn't see a quick
    way to utilize this in a fresh local install. Thoughts?

@hdub-tech hdub-tech requested a review from a team as a code owner June 5, 2024 04:30
@hdub-tech hdub-tech requested review from priyamsahoo and shatakshiiii and removed request for a team June 5, 2024 04:30
@ssbarnea ssbarnea added the bug label Jun 5, 2024
@ssbarnea
Copy link
Member

ssbarnea commented Jun 5, 2024

@hdub-tech Thanks for the PR. I am not yet sure if adding tox itself to test deps is a good idea or not but I will let it in. Time will tell.

@ssbarnea ssbarnea merged commit aa34420 into ansible:main Jun 5, 2024
25 of 26 checks passed
@hdub-tech hdub-tech deleted the enhancement/contributing branch June 5, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants