-
Notifications
You must be signed in to change notification settings - Fork 119
Switch CI to Github Actions #125
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
Conversation
.github/workflows/test.yml
Outdated
@@ -6,6 +6,7 @@ on: | |||
push: | |||
branches: | |||
- master | |||
- github_actions |
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 any reason to limit branches that run tests at all? It results in ugly hacks like what you had to do in this commit.
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.
This is just because github actions is not yet enabled in this repository, otherwise CI would've run because of the pull_request event. The reason to limit branches is to avoid duplicate CI runs on pushes to pull requests.
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.
Even after GitHub Actions is enabled, user might want to run tests before creating a PR.
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. This is maintainers preference. My preference is that users either open a WIP PR or modifiy the workflow temporarily, and avoid all the duplicate builds by default. Others may have other preference. It's no big deal, I can change this to whatever @drbrain prefers.
For some reason, Windows builds pass in your branch even though there was an error in tests. |
Yeah, bug in |
I'm not sure. See my run. The only difference is that you combined |
Right, that's because the default Windows shell doesn't seem to have an |
I suggest splitting Windows CI into a separate PR. This is not a regression (net-http-persistent never had Windows CI before), but it severely increases amount of changes. And it might be that net-http-persistent test suite simply doesn't work on Windows and needs even more fixes. |
Because it's PowerShell) However, |
Yeah, I splitted the tests like you did 👍.
Agreed, but I want to get everything working first. I can split it afterwards if maintainers prefer so. This repo is barely attended so I feel relying on many dependant PRs might make things slower and less likely to get attention 🤷♂️. |
I managed to repro the issue, although I had to add a bunch of patches to dependent libraries. I guess I'll start by submitting patches upstream. |
That might take some time before upstream libs accept changes and make releases with them. That's why I suggested splitting: travis-ci.org that is currently used for CI is going to stop existing on December, 31. So moving to a different CI has a value on its own. |
Sure, I'm not saying no to your suggestion, I'm just waiting for some input from maintainers while working on reproducing and fixing the current issue in CI. As soon as maintainers tell me what they expect, I can quickly amend this PR :) |
It's all green now, by the way https://github.com/deivid-rodriguez/net-http-persistent/runs/1572593796?check_suite_focus=true. |
d232761
to
8e317fc
Compare
Made some progress here by completely getting rid of |
23b8a67
to
51f53a0
Compare
I didn't find time to follow up on this yet, but at least I created a WIP PR to hoe to see how maintainers there feel about my patches. |
Both my PR and the PR to hoe adding a CI have been unattended for a while. Maybe an alternative would be to move away from |
@deivid-rodriguez To keep this up to date, can you add '3.0' (note the quotes) and 3.1 to the CI matrix? |
It doesn't seem necessary and we're moving away from TravisCI.
51f53a0
to
dd68da9
Compare
Done, hopefully this repository gets some love again. |
It's easier to manage the `Gemfile` directly.
Changelog seems managed directly, and manifest is checked by `rake check_manifest`.
I don't see where it's used.
Same thing, but https.
94aa6ef
to
d2c3332
Compare
I want to add a little gem to manage the Rakefile to replace Hoe (which does not support Windows). But this gem does not support Ruby 2.3. The easiest fix is to drop Ruby 2.3 support.
Instead, use explicit gemspec + `Rake::Manifest::Task` + `Rake::TestTask`.
d2c3332
to
32a63ce
Compare
Alright, I updated this PR so it doesn't depend on any fixes in Hoe. Now it does the following: Development Environment
User facing changes
I'll set it as ready! |
❤️ |
Closes #124.
Fixes #129.