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

Upgrade sops to go 1.13 #566

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Upgrade sops to go 1.13 #566

merged 1 commit into from
Nov 18, 2019

Conversation

ajvb
Copy link
Contributor

@ajvb ajvb commented Nov 6, 2019

Upgrade sops to go 1.13, making proper use of go modules internally - https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher

fyi @g-k - Until this is merged into master and released, sops cannot be imported using go modules in go 1.13

Supersedes #531
Relates to Homebrew/homebrew-core#44191

@ajvb ajvb requested a review from autrilla November 6, 2019 23:30
Copy link
Contributor

@autrilla autrilla left a comment

Choose a reason for hiding this comment

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

lgtm. I have the impression this is going to break a lot of people :( but I guess we have to rip off the bandaid at some point.

README.rst Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 6, 2019

Codecov Report

Merging #566 into develop will increase coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #566      +/-   ##
===========================================
+ Coverage    36.94%   37.11%   +0.17%     
===========================================
  Files           21       21              
  Lines         2891     2891              
===========================================
+ Hits          1068     1073       +5     
+ Misses        1728     1724       -4     
+ Partials        95       94       -1
Impacted Files Coverage Δ
decrypt/decrypt.go 0% <ø> (ø) ⬆️
pgp/keysource.go 32.43% <ø> (ø) ⬆️
config/config.go 71.42% <ø> (ø) ⬆️
sops.go 56.36% <ø> (ø) ⬆️
aes/cipher.go 67.28% <ø> (ø) ⬆️
stores/ini/store.go 29.32% <ø> (ø) ⬆️
gcpkms/keysource.go 24.67% <ø> (ø) ⬆️
stores/dotenv/store.go 31.85% <ø> (ø) ⬆️
azkv/keysource.go 18.47% <ø> (ø) ⬆️
kms/keysource.go 49.04% <ø> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00ded41...8c21479. Read the comment docs.

@autrilla
Copy link
Contributor

autrilla commented Nov 6, 2019

My knowledge about Go modules is pretty much zero, but is there a way we could separate the versioning of the Go codebase from the versioning of the sops binary, which is effectively the only thing we actually version with semver? If we do that, could we call this 'v1' of the codebase and not have it in the import path? Or do you have the have the 'v1' there anyway?

@ajvb
Copy link
Contributor Author

ajvb commented Nov 6, 2019

My knowledge about Go modules is pretty much zero, but is there a way we could separate the versioning of the Go codebase from the versioning of the sops binary, which is effectively the only thing we actually version with semver? If we do that, could we call this 'v1' of the codebase and not have it in the import path? Or do you have the have the 'v1' there anyway?

The thing I'm not really sure on is how changing the go.mod at the root of the repo exactly impacts the sops binary. Using go 1.13, go get -u go.mozilla.org/sops/cmd/sops works exactly as you'd expect it to right now. I'm curious if with this change we would actually need to update it to go get -u go.mozilla.org/sops/v3/cmd/sops or whether go get -u go.mozilla.org/sops/cmd/sops would continue to work as expected.

Additionally, forgot about #540, will want to include that in this PR.

@autrilla
Copy link
Contributor

autrilla commented Nov 6, 2019

I see. I have to think about this a bit more, I'll try to do that over the weekend, but feel free to merge to develop for now.

Basically, I think there's two things we need to version here:

  1. go.mozilla.org/sops/cmd/sops
  2. go.mozilla.org/sops/decrypt

And everything else should be unversioned if we can get away with it. As I said, I'll look and see if this is actually possible. A quick read of go modules makes me think it's not. Perhaps if they were on different repos.

README.rst Outdated Show resolved Hide resolved
@ajvb
Copy link
Contributor Author

ajvb commented Nov 7, 2019

@autrilla From what I can tell from my experiments it seems like this might cause only minor breakage for some users.

One concern that I had was whether this could require all users using the sops/decrypt module within their codebases to start using go modules. This does not seem to be the case, surprisingly:

root@18591ccbf56d:/go/t# go version
go version go1.9.7 linux/amd64

root@18591ccbf56d:/go/t# cat ../src/github.com/ajvb/modules-testing-sops/go.mod              
module github.com/ajvb/modules-testing-sops/v2

go 1.13

root@18591ccbf56d:/go/t# cat ../src/github.com/ajvb/modules-testing-sops/anotherfile.go 
package modules_testing_sops

import "fmt"

func AnotherExampleFunc(msg, secondMsg string) {
        fmt.Println(msg)
        fmt.Println(secondMsg)
}

root@18591ccbf56d:/go/t# cat ../src/github.com/ajvb/modules-testing-sops/submodule/example.go 
package submodule

import "github.com/ajvb/modules-testing-sops/v2"

func SubmoduleFunc(msg string) {
        modules_testing_sops.AnotherExampleFunc(msg, msg)
}

root@18591ccbf56d:/go/t# cat main.go 
package main

import "github.com/ajvb/modules-testing-sops/submodule"

func main() {
submodule.SubmoduleFunc("test")
}

root@18591ccbf56d:/go/t# go run main.go 
test
test

Though the above is seemingly using code with import "github.com/ajvb/modules-testing-sops/v2" and is running on go 1.9, it seems to work as expected without having the specify the v2 itself.

As well, go get to run the binary (or more explicitly, outside the context of a go module) cannot make use of versions, so you should be able to still run go get -u go.mozilla.org/sops/cmd/sops and get master.

I think where this might cause stuff to break is that people who have sops with a commit pin in their go.mod like go.mozilla.org/sops v0.0.0-20190611200209-e9e1e87723c8 and want to use it with go 1.13 will need to change it to go.mozilla.org/sops/v3 v3.4.1 or similar.

Please feel free to double check me as I still feel very new to go modules, but I think we are in the relative clear for breakage.

Lastly, after thinking about it a bit, I think that versioning the decrypt module and the sops binary separately would be a major change in how we structure the sops project and I don't quite see the value as long as we can reduce the breakage for our users. Keeping the binary and modules in sync version wise allows for very clear expectations on what versions of the binary or various modules will work with each other. Having a case where versions 1.X of the decrypt module work with 3.X through 4.4 of the binary or something could create a lot of confusion, both for us and for our users.

Copy link
Contributor

@carlpett carlpett left a comment

Choose a reason for hiding this comment

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

Looking forward to this! Noticed one thing to check

go.mod Outdated Show resolved Hide resolved
@autrilla
Copy link
Contributor

@ajvb thanks a lot for explaining. I am very surprised things still work for older versions of Go, so since that's the case, I have no objections to merging this whatsoever!

@ajvb
Copy link
Contributor Author

ajvb commented Nov 18, 2019

@autrilla Of course! Same hehe and sounds good.

@ajvb ajvb force-pushed the upgrade-to-go-1.13 branch 2 times, most recently from 06aa53c to d917a74 Compare November 18, 2019 17:41
@@ -10,7 +10,7 @@ all: test vet generate install functional-tests
origin-build: test vet generate install functional-tests-all

install:
$(GO) install go.mozilla.org/sops/cmd/sops
$(GO) install go.mozilla.org/sops/v3/cmd/sops
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carlpett Not changing this was the cause of the addition of the old sops path to go.mod.

@ajvb ajvb merged commit 8e21de8 into develop Nov 18, 2019
@ajvb ajvb deleted the upgrade-to-go-1.13 branch November 18, 2019 18:07
This was referenced Jan 30, 2020
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.

None yet

5 participants