Resolve "GitLab Runner http endpoint should default to 9252"

This is a general Merge Request template. Consider to choose a template from the list above if it will match your case more.

What does this MR do?

Defaults the MetricsServer Port to 9252 as discussed in #2454 (closed).

Why was this MR needed?

#2454 (closed)

Are there points in the code the reviewer needs to double check?

I don't really like the way I had to handle the error of net.SplitHostPort, maybe you have a better idea on how to do that. The only alternative i can think of is maybe using url.Parse instead.

Does this MR meet the acceptance criteria?

• Documentation created/updated
• Tests
  • Added for this feature/bug
  • All builds are passing
• Branch has no merge conflicts with master (if you do - rebase it please)

What are the relevant issue numbers?

Closes #2454 (closed)

changed the description

changed the description

marked the checklist item All builds are passing as completed

@Rukenshia There are no tests that are checking what will happen if invalid value will be passed (the metricsServerAddress() returns now an error on the second place). There are also no tests that are checking what will happen when the setting will be empty. It should return an empty string (which will then disable metrics server in setupMetricsAndDebugServer()) and now it returns :9252 which will start the server (on 0.0.0.0 which is even worse because it's by default open to whole world)!

Please add such tests:

func TestMetricsServerNone(t *testing.T) {
	cfg := configOptionsWithMetricsServer{}
	cfg.config = &common.Config{}

	address, err := cfg.metricsServerAddress()
	assert.NoError(t, err)
	assert.Equal(t, address, "")
}

func TestMetricsServerEmpty(t *testing.T) {
	cfg := configOptionsWithMetricsServer{}
	cfg.config = &common.Config{}
	cfg.MetricsServerAddress = ""

	address, err := cfg.metricsServerAddress()
	assert.NoError(t, err)
	assert.Equal(t, address, "")
}

func TestMetricsServerEmptyCommonConfig(t *testing.T) {
	cfg := configOptionsWithMetricsServer{}
	cfg.config = &common.Config{}
	cfg.config.MetricsServerAddress = ""

	address, err := cfg.metricsServerAddress()
	assert.NoError(t, err)
	assert.Equal(t, address, "")
}

func TestMetricsServerInvalid(t *testing.T) {
	cfg := configOptionsWithMetricsServer{}
	cfg.config = &common.Config{}
	cfg.MetricsServerAddress = "localhost::1234"

	address, err := cfg.metricsServerAddress()
	assert.Error(t, err)
	assert.Equal(t, address, "")
}

func TestMetricsServerInvalidCommonConfig(t *testing.T) {
	cfg := configOptionsWithMetricsServer{}
	cfg.config = &common.Config{}
	cfg.config.MetricsServerAddress = "localhost::1234"

	address, err := cfg.metricsServerAddress()
	assert.Error(t, err)
	assert.Equal(t, address, "")
}

And add fixes that will make the code pass this tests :)

I really messed that one up. Sorry for that, forgot to check that the address should not be empty. I've also added the tests you provided @tmaczukin.

resolved all discussions

changed the description

resolved all discussions

@Rukenshia Last comment left. Please update this and I'm merging the MR :)

I've changed it and also included an additional test (port-set-without-address) @tmaczukin.

resolved all discussions

Thanks @Rukenshia!

approved this merge request

merged