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

Notary 0.6.1 pulled via go.mod has a bug as well the upcoming 0.6.2 release #1533

Open
marcofranssen opened this issue Feb 6, 2020 · 3 comments

Comments

@marcofranssen
Copy link
Contributor

marcofranssen commented Feb 6, 2020

Following error is shown when trying to run some test utilizing this util.

/Users/marco/go/pkg/mod/github.com/theupdateframework/notary@v0.6.1/utils/http.go:112:24: not enough arguments in call to challenge.SetHeaders
	have (http.ResponseWriter)
	want (*http.Request, http.ResponseWriter)

The fix is already on master, so probably needs a hotfix on the 06.1 release as this blocks people installing the latest release via Go modules.

https://github.com/theupdateframework/notary/blob/master/utils/http.go#L112

When running on master following error occurs. This means the master branch and 0.6.2 release is also having a bug.

Updated using go get github.com/theupdateframework/notary@master.

# github.com/theupdateframework/notary/server
/Users/marco/go/pkg/mod/github.com/theupdateframework/notary@v0.6.2-0.20200114084303-f255ae779066/server/server.go:127:9: undefined: prometheus.InstrumentHandlerWithOpts
/Users/marco/go/pkg/mod/github.com/theupdateframework/notary@v0.6.2-0.20200114084303-f255ae779066/server/server.go:235:44: undefined: prometheus.Handler

It happens when running following code in my own solution test

func fullTestServer(t *testing.T) *httptest.Server {
	// Set up server
	ctx := context.WithValue(context.Background(), notary.CtxKeyMetaStore, storage.NewMemStorage())

	// Do not pass one of the const KeyAlgorithms here as the value! Passing a
	// string is in itself good test that we are handling it correctly as we
	// will be receiving a string from the configuration.
	ctx = context.WithValue(ctx, notary.CtxKeyKeyAlgo, "ecdsa")

	// Eat the logs instead of spewing them out
	// var b bytes.Buffer
	// l := logrus.New()
	// l.Out = &b
	// ctx = ctxu.WithLogger(ctx, logrus.NewEntry(l))

	cryptoService := cryptoservice.NewCryptoService(trustmanager.NewKeyMemoryStore(passphraseRetriever))
	server := httptest.NewServer(server.RootHandler(ctx, nil, cryptoService, nil, nil, nil))
	config.RemoteServer.URL = ts.URL
	
	return server
}
@marcofranssen marcofranssen changed the title Notary 0.6.1 pulled via go.mod has a bug Notary 0.6.1 pulled via go.mod has a bug as well the upcoming 0.6.2 release Feb 6, 2020
@marcofranssen
Copy link
Contributor Author

@justincormack @thaJeztah

@marcofranssen
Copy link
Contributor Author

I have also tried to use the #1523 branch by using a replace statement in the Go.mod

This is having the same issue as the master branch, which was about to be expected as the dependencies are exactly the same.

I expect this to be easily resolveable if we can upgrade some modules using go.mod

@marcofranssen
Copy link
Contributor Author

How to reproduce:

go mod init github.com/somenotary/integration

service_test.go

package main

import (
	"context"
	"net/http/httptest"
	"testing"

	"github.com/theupdateframework/notary"
	"github.com/theupdateframework/notary/cryptoservice"
	"github.com/theupdateframework/notary/server"
	"github.com/theupdateframework/notary/server/storage"
	"github.com/theupdateframework/notary/trustmanager"
)

func fullTestServer(t *testing.T) *httptest.Server {
	// Set up server
	ctx := context.WithValue(context.Background(), notary.CtxKeyMetaStore, storage.NewMemStorage())

	// Do not pass one of the const KeyAlgorithms here as the value! Passing a
	// string is in itself good test that we are handling it correctly as we
	// will be receiving a string from the configuration.
	ctx = context.WithValue(ctx, notary.CtxKeyKeyAlgo, "ecdsa")

	// Eat the logs instead of spewing them out
	// var b bytes.Buffer
	// l := logrus.New()
	// l.Out = &b
	// ctx = ctxu.WithLogger(ctx, logrus.NewEntry(l))

	cryptoService := cryptoservice.NewCryptoService(trustmanager.NewKeyMemoryStore(passphraseRetriever))
	server := httptest.NewServer(server.RootHandler(ctx, nil, cryptoService, nil, nil, nil))
	config.RemoteServer.URL = ts.URL

	return server
}

func TestFullTestServer(t *testing.T) {
	ts := fullTestServer(t)
	// my custom code utilizing the TestServer for unittesting
}

Now run the test:

$ go test ./...
# github.com/theupdateframework/notary/utils
../../../go/pkg/mod/github.com/theupdateframework/notary@v0.6.1/utils/http.go:112:24: not enough arguments in call to challenge.SetHeaders
        have (http.ResponseWriter)
        want (*http.Request, http.ResponseWriter)
FAIL    github.com/philips-internal/actions-runner-orchestrator [build failed]
FAIL

Now try the same by upgrading to latest master branch as a dependency

$ go get github.com/theupdateframework/notary@master
go: finding github.com/theupdateframework/notary master
$ go test ./...
# github.com/theupdateframework/notary/server
../../../go/pkg/mod/github.com/theupdateframework/notary@v0.6.2-0.20200114084303-f255ae779066/server/server.go:127:9: undefined: prometheus.InstrumentHandlerWithOpts
../../../go/pkg/mod/github.com/theupdateframework/notary@v0.6.2-0.20200114084303-f255ae779066/server/server.go:235:44: undefined: prometheus.Handler
FAIL    github.com/philips-internal/actions-runner-orchestrator [build failed]
FAIL

This last error is also the case when using the version using Go modules from this PR #1523

@justincormack is this more clear?

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

No branches or pull requests

1 participant