Added unit test which covers imagename creation with registries on non default ports. #782
Conversation
|
@aidun please can you fix the CI failure? You should have seen this when the PR was raised, but if you missed it, pleased take a look. Also the commit message is truncated for the commit subject, I think we've spoken about this before, but if in doubt, see the contributing guide for how to wrap the commit message. https://github.com/openfaas/faas/blob/master/CONTRIBUTING.md#commit-messages |
|
This is a PR to show the tests are failing. You ask me to do it. Because of this it is a draft. I this is okay, I have to fix the function. Then I will provide a polished PR. Maybe I miss understood you on slack. |
|
OK no problem. Are you planning on then going on to fix the code, now that you have a failing test? :-) Alex |
|
I will provide a fix in the next days. |
|
@alexellis I provided a fix for the issue. I dont feel comfortable with it, because I found no "good" way to check it. Maybe you or someone other can provide feedback on this. |
|
@viveksyngh a review has been requested. |
|
Sorry that you haven't had a review yet. Let me see if @LucasRoesler has some time to take a quick look. The unit test looks good, but do we have a unit test that checks that the old behaviour still works? (we may already have it) Lucas I think we were just looking for some advice on the style / approach on checking the implementation. |
|
@aidun you mention that you are unhappy with the solution. Can you explain what you don't like about it? |
|
|
||
| func Test_BuildImageName_RegistryWithPort(t *testing.T) { | ||
| want := "registry.domain:8080/image:latest" | ||
| got := BuildImageName(DefaultFormat, "registry.domain:8080/image", "ef384", "master") |
There was a problem hiding this comment.
I think it is probably a good idea to also include one case with a port and a tag, e.g. registry.domain:8080/image:foo
There was a problem hiding this comment.
I pushed a new version with that. Thank you for the hint.
|
@LucasRoesler I don't feel comfortable with string operations and try to avoid such implementations. If you think it is okay, everything is fine. |
- Added unit test which covers imagename creation with registries on non default ports. - Added fix for BuildImageName on registries on non default ports Signed-off-by: Markus Hartmann <markush1986@gmail.com>
|
@LucasRoesler can you take a fresh look at this please? |
Added unit test which covers imagename creation with registries on non default ports.
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist:
git commit -s