Fix intention of support for fallback protocol and add tests for probe/cri/registry #3392
Conversation
gotjosh
force-pushed
the
add-tests-to-probe-cri
branch
from
36424df to
9b99cf9
Compare
|
@bboreham gentle nudge (I completely understand if you've been or still are short on time) As I'd like to try and add some tests for the reporter but I'd like to know whenever I'm on the right track or not. |
|
This generally LGTM, but I'm not actively contributing to scope, but considering this change is adding a test that was previously missing, it looks like a good one to me! But I'm not very sure why CI is failing with an error message that doesn't seem to relate to changes here in any way: Could you rebase and force re-running the CI that way? |
gotjosh
force-pushed
the
add-tests-to-probe-cri
branch
from
9b99cf9 to
3cc641d
Compare
|
@errordeveloper have rebased now but seems to be the same error. |
|
The integration tests only run on branches in the main repo, to avoid exposing credentials to all comers. |
|
@gotjosh The code looks ok, but clearly it is doing more than adding a test (and a refactor), so rather than have me guess what that is, could you state it? I imagine it relates to what you are saying about protocols, but it's better all round if you change the commit message to say what you fixed. |
|
Hey, @bboreham thanks for taking a look and the feedback! 🙌 Are there any particular lines/sections that you would like to document/state as part of the commit message? As part of the PR body, I tried to convey the intention of the first commit in detail perhaps it's just a matter of adding some bits from the description into the commit message. Another thing that occurs to me is that I could split the tests and the changes on two different commits, I'll get tha sorted straight away. The main reasoning behind the change is:
|
Unhappy path tests try to cover three scenarios: - When the endpoint URL scheme is not explicitly supported e.g. HTTP - When the endpoint URL scheme is TCP which is also not supported - When the fail to parse the given URL (to extract the scheme) The happy path covers two scenarios: - When we specify the supported scheme in the URL which is an unix socket e.g. unix///var/run/dockershim.sock - When we pass a socket address but fail to specify the scheme but our registry attempts to use the fallback protocol e.g. var/run/dockershim.sock
When we receive an endpoint address without a protocol, our code states we don't support them and that the format is deprecated. In reality it was not the case, e.g: When we received an address in the form of `127.0.0.1`, we'd attempt to parse the scheme from it, we'd realise is does have one (would be equivalent to "") and our function `parseEndpoint` would return `"", "", fmt.Error`. Then, our `parseEndpointWithFallbackProtocol` would use the fallback protocol (unix) and attempt to connect to `unix://127.0.0.1`. This meant two things: 1. The error returned from `parseEndpoint` would never be thrown 2. We would connect anyways since the address is valid This commit changes the assertion logic to match the intention of using a fallback protocol when one is not supplied.
Also, extracts strings into constants
gotjosh
force-pushed
the
add-tests-to-probe-cri
branch
from
3cc641d to
fbb0277
Compare
gotjosh
changed the title
gotjosh
changed the title
|
Hi @gotjosh Changes looks good to me. I have triggered the integration tests for latest commits. |
Hey all 👋,
First of all, thank you for making such a great project! Found it via Hacktoberfest and immediately jumped into the bandwagon for helping out. Love the current organisation/labelling of the issues. Super helpful for us newcomers.
Figured I should get this PR out sooner rather than later to start getting some feedback. While stressing the different code paths with the tests, I noticed there was one case that no matter what I do, I couldn't get an error to bubble up: The case where the scheme is
"".I'm not sure if this is some sort of 🐞 or am I missing something (?), but I ended up refactoring (to the best of my very limited Go knowledge and understanding of the project) some bits to make it so that it complies with what I think the intention was.
From what I read, the assumption was that we don't support URLs were the scheme is not specified but we're supporting them anyways when we use the fallback protocol. The branch is the more natural thing to miss, so I optimised for supporting the case (feedback is very very welcome).
The last commit is very optional, I'm not familiar with Go (in fact, this is my first PR ever). I'm happy to take it out if this is not the way we do things in Goland.
As for the tests they cover:
Unhappy path tests try to cover three scenarios
The happy path covers two scenarios
socket e.g.
unix///var/run/dockershim.sockto use the fallback protocol e.g.
var/run/dockershim.sockSolves #3302