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 basic GitHub Actions configuration #165

Merged
merged 7 commits into from
May 12, 2021
Merged

Conversation

lthibault
Copy link
Collaborator

As per previous discussion, here is a slightly modified version of the default Go configuration for GitHub Actions. It makes the following changes:

  • Run actions against Go 1.8 (since transport behavior changes slightly as of 1.9, and we have tests for this)
  • Run actions for pushes & PRs. in v3 branch, since most work is taking place there for the time being.

@lthibault lthibault requested a review from zombiezen February 22, 2021 00:16
@lthibault lthibault self-assigned this Feb 22, 2021
@lthibault
Copy link
Collaborator Author

Looks like we might need to move to go modules in order for this to work. @taylorjdawson, is this something I could assign to you?

@taylorjdawson
Copy link

@lthibault yes please!

@lthibault lthibault assigned taylorjdawson and unassigned lthibault Feb 28, 2021
@taylorjdawson
Copy link

@lthibault What do you envision for this PR? I am thinking we should get rid of all top lvl .go files and put everything into either a src folder or another name. Then we want to make a go module out of every subfolder. For instance, rpc would be its own module like module github.com/capnproto/go-capnproto2/rpc

Let me know how that sounds

@lthibault
Copy link
Collaborator Author

This is to be the first step in the cleanup that Ross brought up, here (in his P.S.). As such, I'd rather keep this PR small and focused. The goal would therefore be to "just" set up GitHub actions for this repository such that go test is run (with the -race flag) upon any push to master or v3.

This should be pretty straightforward. The only intermediate step I see is that GitHub Actions seems to require Go projects to have a go.mod. This should be a trivial matter of running go mod init <...>, however you should read up on Go modules, and make sure we aren't painting ourselves into any corners with respect to compatibility.

So basically, the steps are:

  1. Add module config to the repo
  2. Add the GitHub actions config to the repo

And that's it!


Regarding the restructuring of the repo, that's something we should discuss in another issue, or in Slack. The community is split around the question of whether top-level .go files are the way to go, or whether public APIs should go in a pkg/ subpackage (either way, src is considered non-idiomatic).

I'm generally in favor of doing a big code-quality sprint on this repo, but I think it would be wise to hold off on that until we get fully oriented, and have some basic safety-nets in place (e.g. improved CI, fixes for currently-broken tests, and perhaps even more test coverage).

@taylorjdawson
Copy link

@lthibault excellent thank you for the information, I do as you said and check with the go docs to make sure everything is in order 👌

@taylorjdawson
Copy link

taylorjdawson commented Mar 3, 2021

@lthibault

❯ go build .
mem.go:11:2: use of internal package zombiezen.com/go/capnproto2/internal/packed not allowed
list.go:8:2: use of internal package zombiezen.com/go/capnproto2/internal/strquote not allowed

is this expected? mem.go and list.go shouldn't be importing from the internal folder should it? Internal Folder spec

.github/workflows/go.yml Outdated Show resolved Hide resolved
.github/workflows/go.yml Outdated Show resolved Hide resolved
.github/workflows/go.yml Outdated Show resolved Hide resolved
.github/workflows/go.yml Outdated Show resolved Hide resolved
@lthibault lthibault requested a review from zenhack May 12, 2021 16:50
@lthibault
Copy link
Collaborator Author

@zenhack Should be good to go. I've removed the _travis directory, but it seems like there might be some additional work involved in disconnecting TravisCI. Is that something you can look into?

@zenhack
Copy link
Contributor

zenhack commented May 12, 2021

@lthibault, I went into travis and disabled builds for this repository; hopefully that will help.

@lthibault lthibault changed the base branch from master to v3 May 12, 2021 18:04
@lthibault lthibault changed the base branch from v3 to master May 12, 2021 18:04
@lthibault
Copy link
Collaborator Author

@zenhack Are we good to merge this?

@zenhack
Copy link
Contributor

zenhack commented May 12, 2021

I think we should get it to pass first. Also, I think you want to delete .travis.yml too.

@lthibault
Copy link
Collaborator Author

lthibault commented May 12, 2021

I think we should get it to pass first. Also, I think you want to delete .travis.yml too.

Ok. I think this requires merging in #169 due to GitHub Actions expecting a Go module, so I'll start with that.

@zenhack
Copy link
Contributor

zenhack commented May 12, 2021

Makes sense -- so we could either wait until that's merged and then hopefully that will fix CI, or we could merge this as-is, and then merge master back into #169 and confirm that CI passes before merging that one. I'm fine with either approach.

@lthibault lthibault added this to the 3.0 milestone May 12, 2021
@lthibault
Copy link
Collaborator Author

lthibault commented May 12, 2021

Makes sense -- so we could either wait until that's merged and then hopefully that will fix CI, or we could merge this as-is, and then merge master back into #169 and confirm that CI passes before merging that one. I'm fine with either approach.

How do you feel about merging v3 into master?

It seems like v3 is probably the stabler of the two at this point, it's now set up as a Go module, and all tests are passing.

@zenhack
Copy link
Contributor

zenhack commented May 12, 2021

We should probably tag another 2.x release before that, and I have some questions regarding what promises we're making re: API stability; there are a number of open issues for 3.0, and some of them require breaking compatiblity:

https://github.com/capnproto/go-capnproto2/milestone/1

@lthibault
Copy link
Collaborator Author

We should probably tag another 2.x release before that

Agreed.

I have some questions regarding what promises we're making re: API stability

What did you have in mind?

My thinking is that stability promises are perfectly captured by SemVer tagging, so users can rely on that if they want to avoid breaking changes.

As a result, we can just merge v3 into master, but not tag a version 3.0.0 until all the breaking changes in the milestone have been implemented.

@zenhack
Copy link
Contributor

zenhack commented May 12, 2021

Fair enough -- particularly since we've changed the import path I guess we can just consider the API unstable until we tag it. But in that case we should probably change the API Compatiblity section in the README to make it clear that master is a not-yet-stabilized version.

@lthibault lthibault changed the base branch from master to feature/v3 May 12, 2021 20:53
@lthibault lthibault merged commit f88021c into feature/v3 May 12, 2021
@lthibault
Copy link
Collaborator Author

N.B.: merged this into feature/v3 (itself a fork of master), which I will be using to stage the merging of v3 into master.

@lthibault lthibault mentioned this pull request May 13, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants