Allow toolbox containers to be created with a custom hostname #1134
Conversation
The new `--hostname` flag is now available when invoking `toolbox create` in order to create a container with a custom hostname. The new flag does not enforce network-resolvability of any custom hostnames, but it does require that the hostname is valid, per RFC 1123. To preserve backward-compatibility, the hostname will still default to "toolbox" if the flag is not specified. Signed-off-by: Buckley Ross <[email protected]> Change-Id: Ib3c0706e3a93a5748453e39998b9893e3e57c305
These tests ensure that the new `--hostname` flag works as intended. Signed-off-by: Buckley Ross <[email protected]> Change-Id: Id7b7775fe73a6600afe2dabb0d905be5ba17bb57
Updated the `toolbox create` manual page to cover the usage of the new `--hostname` flag for specifying custom hostnames when creating new containers. Signed-off-by: Buckley Ross <[email protected]> Change-Id: I16d7f63769c17aec1ff69d34ac8fe28f48827777
nievesmontero
force-pushed
the
feat/create-hostname
branch
from
38721ae to
200abab
Compare
|
Build failed. ✔️ unit-test SUCCESS in 7m 18s |
There was a problem hiding this comment.
The tests are failing:
not ok 25 create: Create a container with a custom hostname ('test.host-name.local') in 530ms
# (from function `assert_success' in file test/system/libs/bats-assert/src/assert.bash, line 114,
# in test file test/system/101-create.bats, line 102)
# `assert_success' failed
#
# -- command failed --
# status : 1
# output : Error: invalid hostname
# --
#
|
Also, there are several issues and pull requests discussing various approaches to improve the host name. For example, did you see #1086 that seems to implement #969 ? I am curious why you picked #1007 I also remember that @allanday had some ideas about the host name, but I can't find a reference right now. |
| @@ -113,6 +121,10 @@ func init() { | |||
| } | |||
|
|
|||
| func create(cmd *cobra.Command, args []string) error { | |||
| // This regex filters out strings which are not valid hostnames, according to RFC-1123. | |||
| // Source: https://stackoverflow.com/a/106223 | |||
| var hostnameRegexp = regexp.MustCompile(`^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])\\.)*([A-Za-z0-9]|[A-Za-z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])$`) | |||
There was a problem hiding this comment.
Do we really need to have this regular expression directly in the code? Can't we use some Go package for it?
This pull request was picked randomly, trying to solve older pull requests. |
|
@nievesmontero I would be partial towards picking the newest PR to merge instead of the oldest one disclaimer: I'm the guy behind #1086 - totally willing to get this merged if you need some help |
I see. Among other things, the tests are failing for this pull request, while they were passing for #1007 |
I see. This can't be solved randomly. :) We have two broad issues. One is about improving the default hostname for the containers (see #969), and the other is about making it easier to visually distinguish between containers (see #98). I suggest going through those two issues, and all the other issues and pull requests linked from them to get a good idea of the current situation. That will give you a good idea of what needs to be done. |
|
I am going to have to close this pull request because the comments from #1007 apply to this one too. |
|
As the author of #1007, I agree with the decision to close both that PR and this one. #1086 seems to be a much simpler solution which addresses my initial issue without over-complicating things. Not to mention that the passage of time appears to have made integrating my original changes significantly more difficult. |
Based on the following pull request #1007