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

go mods #107

Closed
wants to merge 15 commits into from
Closed

go mods #107

wants to merge 15 commits into from

Conversation

starkers
Copy link

I wanted to bosh on this a little but before I start I've updated it to go mods incase thats handy?

CONTRIBUTING.md Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Makefile Outdated
@@ -3,7 +3,7 @@ include .env

BINARY := wave
VERSION := $(shell git describe --always --dirty --tags 2>/dev/null || echo "undefined")
ECHO := echo
ECHO := echo -e
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC echo -e doesn't work on all platforms, are you working on linux or mac?

Copy link
Author

Choose a reason for hiding this comment

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

Ohh right, yes I recall something similar actually.. (I'm on linux)

If osx has an ancient bash maybe its echo -e won't work?

Can you test for me (assuming you're on osx?)
If it doesn't work, Try adding this to the Makefile: SHELL := /usr/bin/env bash (to ensure echo -e is calling the bash echo)

Otherwise I think we have some options:

  1. drop colour prints, instead be clear using old school tricks like: =========== message =============
  2. I can set the colours via tput
  3. emojis? ¯_(ツ)_/¯

I'll give tput a quick test in meantime

Copy link
Author

@starkers starkers Jun 26, 2021

Choose a reason for hiding this comment

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

according to stack overflow printf works on macOS

So I've updated it and it tests OK on linux (needed to add a \n to the end of each statement)

image

Can you test for me?

Copy link
Collaborator

@JoelSpeed JoelSpeed 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, will try to find a moment to test the printf later today

Makefile Outdated
@ $(ECHO) "\033[36mPuling dependencies\033[0m"
$(DEP) ensure --vendor-only
@ $(ECHO) "\033[36mPuling dependencies\033[0m\n"
go get -v github.com/wave-k8s/wave/cmd/manager
Copy link
Collaborator

Choose a reason for hiding this comment

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

go mod download -x?

Copy link
Author

Choose a reason for hiding this comment

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

ohh right, I just tested and noticed that the Makefile's need some love so I'm working on them.. will take some testing but I hope to get generation +kustomize and as much as I can validated

@starkers
Copy link
Author

Lots of updates done to the Makefiles to get everything working done

Stuck at go generate now as its attempting to pull content from the ../../vendor dep path.. I'll have to dig into this further to see how they're handling that in more recent versions of kube builder (I've not done much generating before)

image

@starkers starkers marked this pull request as draft June 28, 2021 13:43
@starkers
Copy link
Author

Converting the PR to a draft as its WIP

@JoelSpeed
Copy link
Collaborator

That issue can be solved by creating a tools.go file that includes the tool dependency, eg following this https://marcofranssen.nl/manage-go-tools-via-go-modules

@github-actions
Copy link

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

@github-actions github-actions bot added the Stale label Sep 27, 2021
@github-actions github-actions bot closed this Oct 4, 2021
@toelke toelke reopened this Feb 12, 2024
@toelke toelke removed the Stale label Feb 12, 2024
@toelke
Copy link
Collaborator

toelke commented Mar 3, 2024

Rebased to let the tests run.

I'll look at fixing the tests later or tomorrow.

@mamccorm
Copy link

Hey @toelke, I think I was able to repro the test failure locally. This seemed to fix it for me:

@Dentrax
Copy link
Contributor

Dentrax commented Mar 28, 2024

I've tried to combine this PR + Mark's but, but I couldn't able to compile the tests with: go test -c ./...:

# sigs.k8s.io/controller-runtime/pkg/log
../../../go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.2/pkg/log/log.go:48:31: cannot use NullLogger{} (value of type NullLogger) as logr.Logger value in argument to NewDelegatingLogger
../../../go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.2/pkg/log/null.go:30:21: cannot use NullLogger{} (value of type NullLogger) as logr.Logger value in variable declaration
../../../go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.2/pkg/log/null.go:48:37: undefined: logr.InfoLogger
../../../go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.2/pkg/log/null.go:54:9: cannot use log (variable of type NullLogger) as logr.Logger value in return statement
../../../go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.2/pkg/log/null.go:59:9: cannot use log (variable of type NullLogger) as logr.Logger value in return statement
../../../go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.2/pkg/log/deleg.go:103:9: cannot use res (variable of type *DelegatingLogger) as logr.Logger value in return statement
../../../go/pkg/mod/sigs.k8s.io/controller-runtime@v0.2.2/pkg/log/deleg.go:116:9: cannot use res (variable of type *DelegatingLogger) as logr.Logger value in return statement
# github.com/go-logr/glogr
../../../go/pkg/mod/github.com/go-logr/glogr@v0.1.0/glogr.go:18:9: cannot use glogger{…} (value of type glogger) as logr.Logger value in return statement
../../../go/pkg/mod/github.com/go-logr/glogr@v0.1.0/glogr.go:127:36: undefined: logr.InfoLogger
../../../go/pkg/mod/github.com/go-logr/glogr@v0.1.0/glogr.go:142:9: cannot use new (variable of type glogger) as logr.Logger value in return statement
../../../go/pkg/mod/github.com/go-logr/glogr@v0.1.0/glogr.go:148:9: cannot use new (variable of type glogger) as logr.Logger value in return statement
../../../go/pkg/mod/github.com/go-logr/glogr@v0.1.0/glogr.go:151:21: cannot use glogger{} (value of type glogger) as logr.Logger value in variable declaration
../../../go/pkg/mod/github.com/go-logr/glogr@v0.1.0/glogr.go:152:12: undefined: logr.InfoLogger
# github.com/go-logr/zapr
../../../go/pkg/mod/github.com/go-logr/zapr@v0.1.0/zapr.go:133:39: undefined: logr.InfoLogger
../../../go/pkg/mod/github.com/go-logr/zapr@v0.1.0/zapr.go:156:9: cannot use &zapLogger{…} (value of type *zapLogger) as logr.Logger value in return statement

@toelke
Copy link
Collaborator

toelke commented Apr 26, 2024

I have merged #145. Thank you so much for the work in this PR!

@toelke toelke closed this Apr 26, 2024
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