https://rancher.com/ logo
Join Slack
Powered by
This message was deleted.
# k3d
a
This message was deleted.
w
Hey! Please go for it. k3d is a pure community driven project. It's not an official SUSE Rancher product and I'm currently the sole maintainer (with way too little time to take care of all open issues). I appreciate any incoming PR 👍
So... Please go for it and let me know if you need any help 👍
m
great, thank you 👍🏻
hello again, i started with familiarizing myself with the codebase, build system, etc. i encountered two issues i wanted to ask about: - this line is evaluated upon every 
make
invocation and, from what i gather, it's unnecessary to populate local cache when the vendor directory is already there. in fact, i don't even see this 
PKG
variable used anywhere in the project unless some make substitution magic causes it to be used. but even if, should it be evaluated eagerly? i had to comment it out locally. - GOFLAGS also seems to be misconfigured considering that the project uses vendoring. i had to change that line to 
-mod=vendor
. or am i missing something about the setup? i managed to build the binary and unit tests pass, so i'll work on the bug now. a question about CI - do you run E2E tests locally before opening a PR, or can GitHub run them against a branch?
@wide-garage-9465 hi, just making sure you saw that message. ;) i've run the E2E tests locally. right now i'm fixing them after introducing the changes that fix the bug. it's not as easy as i thought given the complex port specification parsing logic which affects several flows, but i think i'll get it done.
w
Didn't see it, sorry!
i had to comment it out locally.
Why did you have to comment it out? I don't think it's required, but it also shouldn't break anything - at least it didn't ever break anything before 🤔
i had to change that line to 
-mod=vendor
.
Did it not work without? I enabled vendoring for convenience as the K8s ecosystem was/is difficult to navigate when it comes to dependencies. It usually wasn't a problem in e.g. the CI environments. Anyway, feel free to include that change (or drop the flag entirely as Go will default to using the vendor dir if it's present) 👍
To be clear: Please put those changes in - they make sense. I was just wondering how they broke the build process for you.
m
Why did you have to comment it out?
not break, but it populates local Go package cache with horrendous amount of data, ignoring the vendor directory. i suppose one might not notice this if their package cache is filled already.
i had to change that line to 
-mod=vendor
.
same with that line, 
readonly
causes the vendor directory to be ignored.
w
Good, I just wanted to make sure that it wasn't actually breaking anything 👎
m
@wide-garage-9465 hi, is it possible to run E2E from a branch on the CI just to check? running them locally i encounter weird issues i think may not be related to my change.
yeah, they're failing even on the 
main
branch
w
Should be possible to run them on your fork or from a PR once you create one
yeah, they're failing even on the main branch
latest commit in 
main
looks fine
m
yes, it must be something with my local environment
i'm getting timeouts on cluster creation 😐
okay, i'll open a PR to run them
@wide-garage-9465 https://github.com/k3d-io/k3d/pull/1550 - could you approve the tests to be run?
w
All tests passed
m
i'm trying to set up a clear VM to see if they pass when run inside it
i'll write unit tests for 
parsePortExposureSpec
, maybe i'll discover an edge case i didn't notice. is something else required to get the PR merged?
w
Sounds good. Nothing else required apart from some time from my side to review 😬
m
i'll let you know when i add the unit tests then 👍🏻
👍 1
@wide-garage-9465 a unit test added, please take a look when you can
w
Sorry for the delay! I just have one question: https://github.com/k3d-io/k3d/pull/1550#discussion_r1952315928
m
you got me scared for a moment! i answered the question
w
😄 I just didn't check back on the surrounding code that was hidden in the PR view, sorry
I kicked of a release v5.8.2
Thanks for your contribution! 🙏
m
🥳
i'll be looking for other issues to fix and getting in touch with you
w
Looking forward to any PR ❤️
Hey @microscopic-jewelry-46002 - seems like that change broke a pretty popular use case: https://github.com/k3d-io/k3d/issues/1552 Do you have the time to take a look?
m
hello, yes i do
working on it
🙏 1
@wide-garage-9465 hi, i can write a test, but can you confirm my approach is correct in the GH issue? because it's something of na open question.
w
I just threw in an answer - lmk what you think
m
so if i understand correctly - no changes on k3d side, rather adding support for 
REGISTRY_HTTP_ADDR
in the ligfx image?
or you mean checking if the registry image is that from ligfx (or default)?
w
If the port syntax behaves just like that, I think we're fine. I.e. I can tell it to listen on port 5000 by specifying it explicitly, right? We just changed the default behavior. (On my phone, but I believe that's what your PR did, right?)
m
yes, the listening port can be specified explicitly. i'm not sure if the current behavior is good for custom image cases, but i'm only discovering this codebase, so i'm not fully sure what's the best choice. if you decide on something, just let me know, i can work on that. i don't want to leave a mess behind.
w
That's no mess, no worries. I'll think about this a little more, but so far I'm on the page that we should do it the way I wrote it in the issue comment. Additionally we can call it out more clearly in the release notes. Maybe even an additional log message that behavior has changed.. or just showing the port mapping it uses in the logs.
👍🏻 1
Seeing that this apparently broke quite a few use cases, I'll revert your change and push v5.8.3 - then we'll re-introduce it together with the new registry variant in a "breaking" change and at least a minor release
We also need to update the docs around the registry in general, as we tell to deploy with the image referencing the registry on port 5000
m
👍🏻
w
https://k3d.io/stable/usage/registries/#using-k3d-managed-registries needs to be updated - at least with a hint that by default, the internal port matches the external one. I.e. that one cannot specify a K3s registry.yml file with a mirrors config in it without explicitly specifying the registry port upon creation.
Copy code
mirrors:
  "<http://docker.io|docker.io>":
    endpoint:
      - <http://k3d-docker-io:5000>
This should really be the only part of the documentation that is affected. Then we also put a note into the release notes and do a v5.9.0 release. I'd like to combine this with the integration of the ligfx/k3d-registry-dockerd into k3d.
2 Views
Open in Slack
PreviousNext
Powered by