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

[WIP] Move all CI/CD to Github Actions #446

Closed
wants to merge 4 commits into from

Conversation

srmsoumya
Copy link
Collaborator

@srmsoumya srmsoumya commented Jan 25, 2022

This PR adds support to lint, format & test the codebase across different python & gdal versions.

Description

We can run different steps as:
tox -e [lint/test/format/flake8]

We are not relying on tox.ini file for running different python environments. That is taken care of using github runners. I commented out the format step as that would modify the codebase.

Fixes #440 [WIP]

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected - these changes will not be merged until major releases!)

How Has This Been Tested?

There is technically no change in the codebase, so I have not added any new testcases.

Checklist:

  • My PR has a descriptive title
  • My code follows PEP8
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new errors
  • I have added tests that prove my fix is effective or that my feature works
  • My PR passes Travis CI tests
  • My PR does not reduce coverage in Codecov

cc @rbavery @vincentsarago

SRM added 3 commits January 25, 2022 09:59
- Remove python-env from tox.ini files, use gh-actions matrix for
running different python versions
- Define tox runs as `tox -e [lint/format/flake8/test]`
Copy link
Collaborator

@rodrigoalmeida94 rodrigoalmeida94 left a comment

Choose a reason for hiding this comment

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

Looks great! It's a bit of shame we can't see the actual github run - @rbavery do you have permissions to give me and @srmsoumya mantainer roles in this repository?

Comment on lines +48 to +49
# - name: Format with tox
# run: tox -e format
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be great to make a separate PR with all the formatting changes so that version 0.5 is formatted

Copy link
Collaborator

Choose a reason for hiding this comment

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

Created this PR #448 . Would be great to merge before this.

.github/workflows/tests.yml Show resolved Hide resolved
- name: Test with tox
run: tox
run: tox -e test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're at it maybe we can also add the workflow to deploy the package to pypi using the ttox build commands?
tox -e build && tox -e release

Maybe we can use this github action with a manual approval step using a "release" environment? https://cloudlumberjack.com/posts/github-actions-approvals/

Would be totally OK if we leave this for another PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think there is build & release script already added to the tox.ini file by @rbavery.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Rodrigo's method sounds like a good option to switch to, I don't think I added the build and release script(it was already there). It'd be great to handle this with a gh action with a manual approval step

Copy link
Contributor

Choose a reason for hiding this comment

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

@srmsoumya do you need pypi credentials to authenticate this manual progress? @avanetten do you have these available and could you share them in an email?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added a release workflow here #449

Copy link
Collaborator

Choose a reason for hiding this comment

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

No Manual step for now but it will only run if you add a tag to the PR that increases the version.

@rbavery
Copy link
Contributor

rbavery commented Jan 26, 2022

@rodrigoalmeida94 @srmsoumya thanks for the heads up, just added you both as admins

@rodrigoalmeida94
Copy link
Collaborator

rodrigoalmeida94 commented Jan 28, 2022

@srmsoumya one thing I noticed with flake8 is the check occurs, and there are 217 errors, but the CI is not failing - trying to figure out why that is.

Also the lint command tox -e lint is only formatting the code (using black and isort, like format), I'll make a branch rewriting that step as lint. #450

- Tested github-actions locally using act.
- osgeo ubuntu-large images are 1.4 GB in size, takes some time to run this locally
- Replacing this with ubuntu-small runs it faster with no side effects
@srmsoumya
Copy link
Collaborator Author

@rodrigoalmeida94 I guess you already figured this out. FWIW, adding runs like a list in tox files returns success code even when it fails the CI check.

commands=
python -m pytest --cov --cov-report xml --cov-report term-missing --ignore=venv
commands =
- python -m pytest --cov --cov-report xml --cov-report term-missing --ignore=venv
Copy link
Collaborator

Choose a reason for hiding this comment

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

this minus should not be here, otherwise this will always return success even if the tests fail

@rodrigoalmeida94
Copy link
Collaborator

@srmsoumya seems the issues has to do with writing commands with -. i.e. this:

[testenv:lint]
basepython = python3
skip_install = true
deps = flake8
commands = 
    - flake8 .

Will always return success (even if there are linter errors).

@srmsoumya
Copy link
Collaborator Author

#451 supersedes this PR

@srmsoumya srmsoumya closed this Feb 2, 2022
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

3 participants