-
Notifications
You must be signed in to change notification settings - Fork 240
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
Move release from travis to github actions #360
Conversation
@BeyondEvil @gnikonorov That is ready to ship! |
I've started the review. But since the GHA is all new to me, it's going to take a while. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick skim now. Overall I think it looks good, I just had one question out of curiosity. I have to step away, but I'll do a proper review when I'm back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, overall looks good, I have the same questions regarding deleting tags though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this will officially deprecate our usage of travis, can you remove any references to it as part of this commit or open an issue after this merges so we don't forget? I see the following references to it in the codebase:
$ git grep travis
README.rst:.. image:: https://img.shields.io/travis/pytest-dev/pytest-html.svg
README.rst: :target: https://travis-ci.org/pytest-dev/pytest-html/
development.rst:All pull requests and merges are tested in `Travis CI <https://travis-ci.org/>`_
development.rst:based on the ``.travis.yml`` file.
development.rst:Usually, a link to your specific travis build appears in pull requests, but if
development.rst:`pull requests page <https://travis-ci.org/pytest-dev/pytest-html/pull_requests>`_
development.rst:8. Done. You can monitor the progress on `Travis <https://travis-ci.org/pytest-dev/pytest-html/>`_
$
Indeed, I forgot the cleanup travis references. I will update the PR. |
.github/workflows/actions.yml
Outdated
# - name: Publish to test.pypi.org | ||
# if: >- | ||
# ( | ||
# github.event_name == 'push' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
# github.event_name == 'push' && | |
# false && | |
# github.event_name == 'push' && |
instead of commenting out the whole thing?
FTR these days I'd rely on https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/ instead of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for cleaning up the docs! I have a few nitpicks, but don't mind approving after @webknjaz's feedback is addressed.
All pull requests and merges are tested in `Travis CI <https://travis-ci.org/>`_ | ||
based on the ``.travis.yml`` file. | ||
All pull requests and merges are tested in `GitHub Actions <https://github.com/pytest-dev/pytest-html/actions>`_ | ||
which are defined inside ``.github`` folder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: 'inside the .github
folder.'
development.rst
Outdated
The only way to trigger Travis CI to run again for a pull request, is to submit | ||
another change to the pull branch. | ||
To retrigger CI to run again for a pull request, you can use either use | ||
dropdown option, close and reopen pull-request or to just update the branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: 'use the dropdown option, close and reopen the pull-request or just update the branch'
FWIW none of my feedback is blocking, it's rather informative. |
Thank you for clarifying @webknjaz! @ssbarnea let me know then when you've addressed all the feedback you wanted to and I'll approve. I'd like to wait for @BeyondEvil to also approve before we merge |
I also think this would be nice, but just as @webknjaz, this not blocking for me. 😁 |
I managed to setup testpypi and I will update this PR tomorrow. |
Hey @ssbarnea can we merge this in first, and then add |
6313926
to
dde51aa
Compare
I had to rebase, fixed additional commend. To avoid delaying this even more lets merge it as soon we get the review and we will do other improvements in follow-ups. |
- create packaging environment that produces deliverables - removes travis integration - configures github actions to call packaging after all the other tests are passing and to make a release if tag is present - test.pypi.org is kept commented until we sort credentials for it - no credentials are needed as they are already configured in github
Of course @ssbarnea! I'll take a look today. Thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
As @ssbarnea mentioned we can get any improvements we need in subsequent PRs. I think this is fine as is and will let us start releasing with the new workflow.
I would like one more person to approve before merge, however. @BeyondEvil can you take a look as well?
- name: Switch to using Python 3.6 by default | ||
uses: actions/setup-python@v2 | ||
with: | ||
python-version: 3.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we're using 3.6 and not a newer version @ssbarnea @gnikonorov ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure Sorin wanted to support the oldest possible version that the devs/contributors may use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, use of oldest supported is usually better than newest in order to avoid regressions. Based on experience using oldest proved to catch more issues than the newest, especially in this area.
Any CI failure that prevents a bad release is better than making a user unhappy. Note to myself: add comments on action tasks like this documenting their purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ssbarnea! Makes sense. 👍
tests are passing and to make a release if tag is present