Skip to content

ci: replace TravisCI with GitHub Actions#483

Merged
markerikson merged 4 commits into
reduxjs:masterfrom
eXamadeus:ci/convert-travis-to-gha
Feb 8, 2021
Merged

ci: replace TravisCI with GitHub Actions#483
markerikson merged 4 commits into
reduxjs:masterfrom
eXamadeus:ci/convert-travis-to-gha

Conversation

@eXamadeus
Copy link
Copy Markdown
Contributor

@eXamadeus eXamadeus commented Feb 8, 2021

Replace TravisCI for GitHub Actions

This pulls inspiration from https://github.com/reduxjs/redux-toolkit/blob/next/.github/workflows/tests.yml and spawned from conversations in #480.

Fixes #482

Notes to check up on:

  • are the number of TS versions correct this is interesting, see the Notes section
  • should we remove some of the older node versions?
  • will this actually work once we merge into the main repo? or do we need to enable actions somehow?

Notes

I was going to update TypeScript to 3.5 (and only test past there) as we discussed in #480 but it looks like there is an interesting bug with TypeScript (or maybe a fix that seems like a bug) where the array typings used by reselect to type the combiner...well just don't work as expected past TS 3.1...but that makes sense. We should really do that in the type update PR anyway (the 5.0 release).

More info...

Upon looking into this, it appears to just have been a change in the way TypeScript resolves the order of inferred type parameters. It checks the combiner function types before enforcing the constraints of the selectors. This means that the types for array-input selectors probably never worked as intended for TypeScript 3.1+.

I have the failures showing here in CI: https://github.com/eXamadeus/reselect/actions/runs/546546429

@eXamadeus eXamadeus marked this pull request as ready for review February 8, 2021 01:06
@eXamadeus eXamadeus force-pushed the ci/convert-travis-to-gha branch from d25edb1 to 375ea7d Compare February 8, 2021 01:08
type: fix failures in TS 3.x+ where "toString doesn't exist on T"

typescript: show failures starting in 3.1

ci: only run against "working" verions of TypeScript
@eXamadeus eXamadeus force-pushed the ci/convert-travis-to-gha branch from 9e64247 to 6f14773 Compare February 8, 2021 01:39
@eXamadeus
Copy link
Copy Markdown
Contributor Author

Let me know what you think about the questions I left up @markerikson and @phryneas

@eXamadeus
Copy link
Copy Markdown
Contributor Author

eXamadeus commented Feb 8, 2021

See passing CI runs here: https://github.com/eXamadeus/reselect/actions

(and all the failing ones from my testing...haha)

@markerikson
Copy link
Copy Markdown
Contributor

Yipes. That's... lovely :)

Obviously my goal here was that we'd do the CI switch, see the current code building okay, and then work on any further updates.

I don't have an immediate suggestion here, tbh. I know Lenz has been kinda burned out lately, so while I'd really like to get his feedback on this, not sure if he's feeling up to it atm.

I'll try putting out the bat-signal on Twitter and see if anyone else has suggestions.

@nickserv
Copy link
Copy Markdown
Contributor

nickserv commented Feb 8, 2021

should we remove some of the older node versions?

Looks like this was already done. I agree with it as only Node 10+ is currently supported.

will this actually work once we merge into the main repo? or do we need to enable actions somehow?

Yup, it should be automatic.

Copy link
Copy Markdown
Contributor

@nickserv nickserv left a comment

Choose a reason for hiding this comment

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

Looks good overall, though I'm not sure if we need both Coveralls and Codecov, they both gather code coverage data.

@eXamadeus
Copy link
Copy Markdown
Contributor Author

eXamadeus commented Feb 8, 2021

Coveralls and Codecov

Yeah I just brought over everything that was in Travis. I didn't want to remove anything without confirmation. Which would be preferred you think? (Edit: I think codecov.io affects PRs, so that might be the one to keep right? I'm not sure it triggered though, I might need more config to get that running)

node looks updated

Also, I'm a dope. I totally changed the node versions already. What I meant to say is "are these the ones we should support" or something haha!

@nickserv
Copy link
Copy Markdown
Contributor

nickserv commented Feb 8, 2021

I'm not sure. It may be safer to leave them both in, unless another maintainer can confirm if only one is needed.

@eXamadeus
Copy link
Copy Markdown
Contributor Author

eXamadeus commented Feb 8, 2021

OK so I think I verified that Codecov.io is uploading correctly: https://codecov.io/github/eXamadeus/reselect/commit/9626793b028b2872bca2a02cc3353ba6c7ad10ad

Although I'm not entirely convinced, because I would have expected a comment on this PR (or a "check")...any ideas how to set that up? The docs for the action don't lead me to believe there is a configuration value I'm missing.

@eXamadeus
Copy link
Copy Markdown
Contributor Author

eXamadeus commented Feb 8, 2021

Hmm, well Codecov comments are working on my "testing" PR: eXamadeus#1

Not sure if this is an issue with forked repos vs "natural branches" on the targeted repo...

EDIT: This is likely just because there is no branch in this repo (reduxjs/reselect) with GitHub Actions enabled. This should work fine when merged (I'm hoping).

@eXamadeus
Copy link
Copy Markdown
Contributor Author

eXamadeus commented Feb 8, 2021

@nickmccurdy Are you familiar with the comment setup for Codecov? I made some small changes to make sure Codecov reports when PRs are made. Let me know if those settings make sense.

I decided to turn off the "master only" filter so branches like v5.0.0 would get coverage checks too. Not sure if that's desirable or not.

@markerikson
Copy link
Copy Markdown
Contributor

Yeah, if the build+test steps are working here, I'm fine with getting this in as a baseline. We can always double-check code coverage behavior later and fix it if necessary.

@markerikson
Copy link
Copy Markdown
Contributor

Skimming the settings, seems reasonable to my not-familiar-with-GH-actions eye. You happy with this as-is?

@markerikson
Copy link
Copy Markdown
Contributor

Okay, let's get this in and keep going, then.

@markerikson markerikson merged commit d889c32 into reduxjs:master Feb 8, 2021
@eXamadeus eXamadeus deleted the ci/convert-travis-to-gha branch February 9, 2021 05:21
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.

Modernize CI setup

3 participants