Skip to content

Conversation

@SVilgelm
Copy link
Contributor

@SVilgelm SVilgelm commented Jan 27, 2020

Fix #23

Run tests using GitHub Actions for all raised pull requests and pushed commits to a branch.
Switching to GitHub Actions give us more flexibility, it allows to run the tests in any fork for push event.

Examples of the Push and Pull Request checks are here: https://github.com/SVilgelm/flask-restx/pull/3/checks
it's a PR to master branch in my fork.

Example of the release workflow: https://github.com/SVilgelm/flask-restx/runs/410117918?check_suite_focus=true
and the pypi: https://pypi.org/project/flask-restx-svilgelm-test/99.99.99/

The workflows need the PYPI_PASSWORD (token) and COVERALLS_REPO_TOKEN secrets to be configured.

@SVilgelm SVilgelm changed the title Run tests using github actions WIP: Run tests using github actions instead of travis Jan 27, 2020
@SVilgelm SVilgelm force-pushed the github-actions branch 2 times, most recently from 09d9863 to ca6234b Compare January 27, 2020 04:00
@SVilgelm SVilgelm changed the title WIP: Run tests using github actions instead of travis Run tests using github actions instead of travis Jan 27, 2020
@SVilgelm SVilgelm changed the title Run tests using github actions instead of travis Using GitHub Actions instead of Travis CI Jan 27, 2020
@SVilgelm
Copy link
Contributor Author

@ziirish @SteadBytes please look at this PR and tell me your thoughts about it.

@SteadBytes
Copy link
Contributor

SteadBytes commented Jan 28, 2020

@SVilgelm, thank you for your efforts here! However I cannot find a related issue and discussion regarding this change - please do open an issue before implementing changes for a PR. This allows discussion around how a change should be made/if it's needed at all to ensure that contributors aren't using up their valuable time needlessly!

I am not familiar with GitHub actions and I haven't personally had any trouble with our Travis CI setup yet so I will need to spend some time evaluating whether this change - Ill do this tomorrow. It should also be discussed by other team members @ziirish, @j5awry.

@ziirish
Copy link
Contributor

ziirish commented Jan 29, 2020

Hello,

Sorry I'm overwhelmed at the moment both professionally and personally, hence my lack of activity the last few days.

I agree with @SteadBytes on the fact we didn't discuss this change earlier (or not deep/long enough). I remember we had a quick chat about such eventuality prior we forked but we didn't settle anything.

Anyway, I never used GitHub actions so I'll need to take some time to properly review your proposal.
But I like the idea, so thanks for your time and effort on this!

@SVilgelm
Copy link
Contributor Author

@SteadBytes I opened the issue: #23, so we can collect opinions.
@ziirish I totally understand, that all of you need a time to review it, to read about GitHub Actions and etc. There is no rush, it is just an enhancement for CI part.

@SteadBytes
Copy link
Contributor

No worries @ziirish - everyone's busy and no one should expect immediate response from anyone!

Thanks for opening the issue @SVilgelm, though my point was more that it's useful to open an issue before working on the PR 😅

@SVilgelm
Copy link
Contributor Author

@SteadBytes, opening an issue means having a discussion and we could decide to not do it, since you already have the working Travis CI. But now you have an examples, how it works, how it looks like and it is easier to make a "right" decision ;-)

@SVilgelm
Copy link
Contributor Author

Anyway I didn't spend too much time on this work. It's just for fun, new challenges, new experience.

@ziirish
Copy link
Contributor

ziirish commented Jan 30, 2020

I have now partially read the github actions doc and I'm confident with your change.
I have also added both required secrets in the repo's settings so I'm okay merging this if there are no blocker with others.

Thanks again for your time and effort.

Copy link
Contributor

@ziirish ziirish left a comment

Choose a reason for hiding this comment

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

I noticed a few possible improvements. I guess I could have had the same remarks on the original .travis-ci.yml file, but since we didn't author this file in the first place, I never gave it a proper review.

Other than that, everything looks clear to me and I'm okay with such change if others agree as well.

Copy link
Contributor

@ziirish ziirish left a comment

Choose a reason for hiding this comment

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

LGTM

@SVilgelm
Copy link
Contributor Author

SVilgelm commented Jan 31, 2020

Here is the checks on my fork: https://github.com/SVilgelm/flask-restx/pull/4/checks

@SVilgelm
Copy link
Contributor Author

I have a couple of Ideas how to Improve it:

But will create additional PR when this is merged

@j5awry
Copy link
Contributor

j5awry commented Feb 1, 2020

Run the test workflow by a cron, on a weekly basis, so it will test the restx with all new dependencies

I don't see a step for this. I think we can open a separate issue related to dependencies, how we're handling them, and testing upgrades. some of the past tickets have been related to Flask versions moving before we've really done a check. Having a cron that automatically runs tests, and runs against newer dependencies would be awesome.

@ziirish
Copy link
Contributor

ziirish commented Feb 1, 2020 via email

@j5awry
Copy link
Contributor

j5awry commented Feb 1, 2020

i've gone ahead and opened #29 for automated dependency checks

@SVilgelm
Copy link
Contributor Author

SVilgelm commented Feb 4, 2020

Guys,
Could anyone describe me how are you doing releases? I just figured out that the release action will be triggered on a tag, but I see you have releases.
@SteadBytes ?

@SteadBytes
Copy link
Contributor

SteadBytes commented Feb 4, 2020

@SVilgelm I'm not quite sure what you mean sorry? Yes releases are performed via Travis whenever a new Git tag is pushed https://github.com/python-restx/flask-restx/blob/master/CONTRIBUTING.rst#release-process

Do you mean the 'releases' on GitHub itself? If so, that's done after the successful push to PyPi from a tag. All we do is manually create a new release (e.g. click the new release button 😂) from the tag and add some release notes. GitHub automatically adds Zip files of the source at the tag.

@SVilgelm
Copy link
Contributor Author

SVilgelm commented Feb 4, 2020

Do you mean the 'releases' on GitHub itself?

yeah, I mean GitHub releases. So, we can make it automatically if you will create a tag with an annotation. I'm doing it here: https://github.com/SVilgelm/marshmallow-objects/releases
If the tag has an annotation it can be used to create a release with the notes: https://github.com/SVilgelm/marshmallow-objects/blob/master/.github/workflows/release.yml#L47

I'm using a tool I have created for such tasks: https://github.com/sv-tools/bumptag

@SVilgelm
Copy link
Contributor Author

SVilgelm commented Feb 5, 2020

So, let’s merge this?

@SVilgelm SVilgelm mentioned this pull request Feb 5, 2020
@SteadBytes
Copy link
Contributor

A few questions first:

  1. I see this builds the released package on Python 3.8 - is this different to what is currently being done and, if so, are there any potential effects of changing it?
  2. What environment variables (e.g. api keys e.t.c) do the maintainers need to specify?
  3. Can the section in CONTRIBUTING.rst be updated with details of this?
  4. Is there a way we can test this by running a build and pushing a release to the PyPi test server?

@SVilgelm
Copy link
Contributor Author

SVilgelm commented Feb 5, 2020

  1. I just chose the 3.8 as latest stable python version, I didn't see any changes, it works well.
  2. As far as I know @ziirish has already added the secrets, just double check that the PYPI_PASSWORD (token) and COVERALLS_REPO_TOKEN exist
  3. Will check
  4. Sure, I have added this information in the PR description:

Examples of the Push and Pull Request checks are here: https://github.com/SVilgelm/flask-restx/pull/3/checks
it's a PR to master branch in my fork.

Example of the release workflow: https://github.com/SVilgelm/flask-restx/runs/410117918?check_suite_focus=true
and the pypi: https://pypi.org/project/flask-restx-svilgelm-test/99.99.99/

I can test it only on my fork. I'm planning to create a PR with something, then we can release version 0.1.1

@SVilgelm
Copy link
Contributor Author

SVilgelm commented Feb 5, 2020

@SteadBytes the contributing.rst is updated

@ziirish
Copy link
Contributor

ziirish commented Feb 7, 2020

I'm still OK with this change.

I have added the secrets in the project settings so hopefully everything should run smoothly.

I'd like this one to be merged prior the v0.1.1 release so we have a real use/test case.

@j5awry
Copy link
Contributor

j5awry commented Feb 7, 2020

I'm still good as well. Would love to see this get in, and the dependency issue. we're seeing fun compatibility issues cropping up.

@ziirish ziirish merged commit e65ae32 into python-restx:master Feb 7, 2020
@SVilgelm SVilgelm deleted the github-actions branch February 7, 2020 21:30
@SVilgelm
Copy link
Contributor Author

SVilgelm commented Feb 8, 2020

Yep, release workflow works well :)))

@ziirish
Copy link
Contributor

ziirish commented Feb 8, 2020

Well done 👍

Now I'll be working on the coveralls issue.

@SVilgelm
Copy link
Contributor Author

SVilgelm commented Feb 8, 2020

What is wrong with coveralls?

@SteadBytes
Copy link
Contributor

Take a look over on Gitter @SVilgelm, I diagnosed the issue early this morning and @ziirish and I looked into it.

@SVilgelm
Copy link
Contributor Author

SVilgelm commented Feb 8, 2020

@thanks, I didn't know about new Gitter channel :)

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.

Using GetHub Actions instead of Travis CI

4 participants