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

Add GitHub Actions for CI #2

Closed
mfridman opened this issue May 22, 2021 · 9 comments · Fixed by #4
Closed

Add GitHub Actions for CI #2

mfridman opened this issue May 22, 2021 · 9 comments · Fixed by #4
Milestone

Comments

@mfridman
Copy link
Member

GitHub Actions probably makes the most sense for OSS projects like this to avoid another dependency on external services.

Setup CI for existing tests on every push

+--------+---------+-------------------------------------+-------+------+------+------+
| STATUS | ELAPSED |               PACKAGE               | COVER | PASS | FAIL | SKIP |
+--------+---------+-------------------------------------+-------+------+------+------+
| PASS   | 0.10s   | github.com/dgrijalva/jwt-go/request | 78.4% |    4 |    0 |    0 |
| PASS   | 0.21s   | github.com/dgrijalva/jwt-go         | 78.3% |   24 |    0 |    0 |
+--------+---------+-------------------------------------+-------+------+------+------+

+--------+---------+-----------------------------------------+---------+
| STATUS | ELAPSED |                  TEST                   | PACKAGE |
+--------+---------+-----------------------------------------+---------+
| PASS   |    0.03 | TestECDSAVerify                         | jwt-go  |
| PASS   |    0.01 | TestParser_Parse                        | jwt-go  |
| PASS   |    0.01 | TestECDSASign                           | jwt-go  |
| PASS   |    0.01 | TestRSAPSSSaltLengthCompatibility       | jwt-go  |
| PASS   |    0.01 | TestParser_ParseUnverified              | jwt-go  |
| PASS   |    0.00 | TestRSASign                             | jwt-go  |
| PASS   |    0.00 | TestNoneSign                            | jwt-go  |
| PASS   |    0.00 | TestNoneVerify                          | jwt-go  |
| PASS   |    0.00 | TestRSAPSSVerify                        | jwt-go  |
| PASS   |    0.00 | TestRSAPSSSign                          | jwt-go  |
| PASS   |    0.00 | TestHMACSign                            | jwt-go  |
| PASS   |    0.00 | TestRSAVerify                           | jwt-go  |
| PASS   |    0.00 | TestHMACVerify                          | jwt-go  |
| PASS   |    0.00 | TestRSAVerifyWithPreParsedPrivateKey    | jwt-go  |
| PASS   |    0.00 | TestRSAWithPreParsedPrivateKey          | jwt-go  |
| PASS   |    0.00 | TestRSAKeyParsing                       | jwt-go  |
| PASS   |    0.00 | ExampleNewWithClaims_standardClaims     | jwt-go  |
| PASS   |    0.00 | ExampleNewWithClaims_customClaimsType   | jwt-go  |
| PASS   |    0.00 | ExampleParseWithClaims_customClaimsType | jwt-go  |
| PASS   |    0.00 | ExampleParse_errorChecking              | jwt-go  |
| PASS   |    0.00 | ExampleNew_hmac                         | jwt-go  |
| PASS   |    0.00 | ExampleParse_hmac                       | jwt-go  |
| PASS   |    0.00 | Example_getTokenViaHTTP                 | jwt-go  |
| PASS   |    0.00 | Example_useTokenViaHTTP                 | jwt-go  |
|        |         |                                         |         |
| PASS   |    0.01 | TestParseRequest                        | request |
| PASS   |    0.00 | TestExtractor                           | request |
| PASS   |    0.00 | ExampleHeaderExtractor                  | request |
| PASS   |    0.00 | ExampleArgumentExtractor                | request |
+--------+---------+-----------------------------------------+---------+
@oxisto
Copy link
Collaborator

oxisto commented May 22, 2021

Not having a go.mod file in the old repo as a reference... which versions do we want to test against?

  • 1.16 obviously
  • It seems the old travis was testing against 1.11, 1.12 and 1.13

@mfridman
Copy link
Member Author

Probably latest of major versions 1.14, 1.15 and 1.16 to follow the Go release cycle .

@lggomez
Copy link
Member

lggomez commented May 22, 2021

Probably latest of major versions 1.14, 1.15 and 1.16 to follow the Go release cycle .

These should be a must since the ideal scenario is to test and validate regressions compatibility against the new and LTE maintained versions (which AFAIK are the current and its 2 previous versions)

* It seems the old travis was testing against 1.11, 1.12 and 1.13

These are the old but current use cases for several users so I guess that decision is up to us. I'm a bit torn on this, the newest 3 versions (and tip as an optional) seems to be the standard for most the big repos I've seen like delve and cobra but compatibility for older versions is something not to be underestimated. I can remember at least 10 projects from my last job that were on 1.12 and were about to debate on whether to upgrade to 1.13 or not... but I also understand that having a CI train of 6 (+1) stages just for the golang versions may be counterproductive for testing and induce flakiness

@oxisto
Copy link
Collaborator

oxisto commented May 23, 2021

but I also understand that having a CI train of 6 (+1) stages just for the golang versions may be counterproductive for testing and induce flakiness

In most cases yes, but since this library has 0 external dependencies and it takes about 14 seconds per Go version (see https://github.com/oxisto/jwt/runs/2647110581), I think we could afford a matrix build of those "older" versions (maybe slowly phasing them out) and them moving along the Go release cycle with current + latest 2.

@lggomez
Copy link
Member

lggomez commented May 23, 2021

but I also understand that having a CI train of 6 (+1) stages just for the golang versions may be counterproductive for testing and induce flakiness

In most cases yes, but since this library has 0 external dependencies and it takes about 14 seconds per Go version (see https://github.com/oxisto/jwt/runs/2647110581), I think we could afford a matrix build of those "older" versions (maybe slowly phasing them out) and them moving along the Go release cycle with current + latest 2.

It's completely reasonable, I like this as a roadmap

@lggomez
Copy link
Member

lggomez commented May 26, 2021

A contributor just added this this PR which does the initial module renaming and while it does not migrate to github actions it updates the travis file with the latest go versions. I asked permission to activate travis on this repo (I guess it went to the creator of this org @mfridman) so we can have it as a temporal CI in the meanwhile, should any unrelated PRs come and follow on with the github actions migration

@oxisto
Copy link
Collaborator

oxisto commented May 26, 2021

A contributor just added this this PR which does the initial module renaming and while it does not migrate to github actions it updates the travis file with the latest go versions. I asked permission to activate travis on this repo (I guess it went to the creator of this org @mfridman) so we can have it as a temporal CI in the meanwhile, should any unrelated PRs come and follow on with the github actions migration

Additionally, we also might decide upon a merging scheme. Looks like the original author did merge commits. I personally am a big fan of squashed merges, because it is just easier to see the actual changes. It's probably personal preference but I guess it should at least be consistent for the new commits.

@lggomez
Copy link
Member

lggomez commented May 26, 2021

A contributor just added this this PR which does the initial module renaming and while it does not migrate to github actions it updates the travis file with the latest go versions. I asked permission to activate travis on this repo (I guess it went to the creator of this org @mfridman) so we can have it as a temporal CI in the meanwhile, should any unrelated PRs come and follow on with the github actions migration

Additionally, we also might decide upon a merging scheme. Looks like the original author did merge commits. I personally am a big fan of squashed merges, because it is just easier to see the actual changes. It's probably personal preference but I guess it should at least be consistent for the new commits.

+1, It's the best github has to offer in this regard and we don't have to bother people with doing interactive rebases 💦

@mfridman
Copy link
Member Author

Also +1 for enforcing squashed merged commits. If it's not already enabled on the repo, I'll make this the default.

@oxisto oxisto added this to the v3.2.1 milestone May 29, 2021
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 a pull request may close this issue.

3 participants