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

[BUGFIX] add default k3s version (honored by go install) #24

Merged
merged 3 commits into from
May 4, 2019

Conversation

cartyc
Copy link
Contributor

@cartyc cartyc commented May 3, 2019

Added a small fix to add the default version for k3s docker image, v0.4.0 as per my issue #23

@ibuildthecloud
Copy link
Contributor

@cartyc So the issue is that the Makefile assigns the k3s version and there is no default in the go source. To set a default in the go source and still respect the Makefile build, you should set the default value here https://github.com/rancher/k3d/blob/60c622e822d3a4d724a42d9e777fddfb3af3325c/version/version.go#L7

Just do

var K3sVersion = "v0.4.0"

@cartyc
Copy link
Contributor Author

cartyc commented May 3, 2019

Thanks @ibuildthecloud . My Go is a little rusty these days, and I didn't consider the makefile. Updated it in the PR.

@zeerorg
Copy link
Collaborator

zeerorg commented May 3, 2019

I think I should cc @iwilltry42 for this since originally we were keeping the k3s version static but shifted that to Makefile.
Maybe we should remove the go install option from readme or we'll have to keep track of both versions.

@iwilltry42
Copy link
Member

I'd like to keep the ability to do a go install since I install many of the tools written in Go from source like this... Surely it's a bit more effort to think of this hardcoded value, but then again it's only a fallback value, so I guess there's no big rush in updating it every now and then.
WDYT @zeerorg ?

@iwilltry42 iwilltry42 changed the title added default version [BUGFIX] add default k3s version (honored by go install) May 3, 2019
@cartyc
Copy link
Contributor Author

cartyc commented May 3, 2019

@iwilltry42 Would a common config file that both the Makefile and Go code could draw from be appropriate in this case? Not sure if that would add or reduce the complexity for the team.

@iwilltry42
Copy link
Member

@cartyc , I guess that wouldn't change too much. The Makefile is supposed to grab the latest version from GitHub. The Go code shouldn't do this dynamically as I'd expect it to also work in airgapped systems.
I'd go for a hardcoded fallback solution or update the readme to include an extra flag like this: go install -ldflags '-X github.com/rancher/k3d/version.K3sVersion=v0.4.0'

@zeerorg
Copy link
Collaborator

zeerorg commented May 3, 2019

Maybe we can set the default tag to "latest" and then show a warning when k3d create is done
Specify k3s version with --version flag

@iwilltry42
Copy link
Member

@zeerorg that sounds like a good idea, but k3s dockerhub repo doesn't use a proper latest tag, as far as I know.

@cartyc
Copy link
Contributor Author

cartyc commented May 3, 2019

Ya that's why I went with the v0.4.0 tag. Using a latest tag would work good too.

@iwilltry42 iwilltry42 requested a review from zeerorg May 3, 2019 18:57
@iwilltry42
Copy link
Member

I'll leave that up to you guys here 🤷‍♂️

@ibuildthecloud
Copy link
Contributor

I just pushed rancher/k3s:latest. Can we switch to rancher/k3s:latest as the default version? Might as well always use latest unless --version is specified.

@cartyc
Copy link
Contributor Author

cartyc commented May 4, 2019

I'll update the PR to use k3s:latest.

@cartyc
Copy link
Contributor Author

cartyc commented May 4, 2019

Did a test deploy and it deployed successfully

▶ k3d create --workers 3
2019/05/03 22:03:58 Created cluster network with ID 1ffd7e6bd03999f4ae60f41ae8676345f5a4168bb323d8c455cd5cf74ce6e290
2019/05/03 22:03:58 Creating cluster [k3s_default]
2019/05/03 22:03:58 Creating server using docker.io/rancher/k3s:latest...
2019/05/03 22:04:00 Booting 3 workers for cluster k3s_default
Created worker with ID acf2855e2c6699989b36f3caf05ca9316fc3aabb2ef1ff3f832e0f815b5ce045
Created worker with ID 3c2afb905f251e00655a7a8e4cc48e052fe6b08df282a32a7d124f34d5da2f52
Created worker with ID a58321a00c9d477c3e276b5db506f8c4318abe714fed76278cea594f117b819c
2019/05/03 22:04:06 SUCCESS: created cluster [k3s_default]

@iwilltry42
Copy link
Member

Cool, thank you 👍
Is updating the latest tag now part of the build pipeline of k3s @ibuildthecloud , then we could really rely on the latest tag completely in the future 👍

@iwilltry42 iwilltry42 merged commit 19c9903 into k3d-io:master May 4, 2019
@cartyc cartyc deleted the default-version branch May 4, 2019 13:22
@cartyc
Copy link
Contributor Author

cartyc commented May 4, 2019

Thanks @iwilltry42 !

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.

4 participants