ci: refactor curl / wget test helpers with retries

[LexCC]

Fixes: #11994

ci: refactor curl / wget test helpers with retries

[coveralls]

Coverage increased (+0.009%) to 36.957% when pulling 7402997 on JieJhih:pr/JieJhih/refactor-test-helpers into d7e2076 on cilium:master.

[aanm]

test-me-please

[nebril]

Thanks a lot for the PR, it was bugging me for quite some time now.

I left a comment inline, please consider changing it as requested.

[nebril]

I think it makes a lot of sense to refactor this, but I feel that this functions shows too many implementation details and nesting CurlFail into CurlWithRetries is not necessary.

I would like CurlWithRetries to accept the same arguments as CurlFail with addition of --retry flag.

I think this function should call CurlFail and then perform string replacement on it that would replace curl with curl --retry %d.

[LexCC]

Thanks for reviewing. I think the function will become something like this:
CurlWithRetries can use failed flag to distinguish CurlFail and non-CurlFail.

func CurlWithRetries(endpoint string, retries int, failed bool, optionalValues ...interface{}) string {
	if failed {
		return fmt.Sprintf(
			`%s --retry %d`,
			CurlFail(endpoint, optionalValues...), retries)
	}
	if len(optionalValues) > 0 {
		endpoint = fmt.Sprintf(endpoint, optionalValues...)
	}
	return fmt.Sprintf(
		`curl --path-as-is -s  -D /dev/stderr --output /dev/stderr --retry %d %s`,
		retries, endpoint)
}

That made this line https://github.com/cilium/cilium/pull/12408/files#diff-0ac1702ef92e08f680c4d7d79842e042R95 can reuse the funtion.

[nebril]

Looks good. One nit on argument names - failed arg should be called fail, as this is equivalent of curl --fail flag.

Also please make sure that adding --retry %d at the end of command always works as intended (although we will probably figure this out when running CI).

[LexCC]

Updated! Please take a look.

[christarazi]

Thanks for the PR! Overall I think this is good. +1 to @nebril's idea to refactor some of the details away in the new function

[christarazi]

LGTM; just a few minor comments, thanks. I think it's worth mentioning that there a few calls that can be simplified (they pass a format string as the 1st arg and utilize optionalValues to fill in), however we can leave that to a follow up.

[christarazi]

test-me-please

[LexCC]

test-me-please

[christarazi]

@jiejhih Only members with commit access can trigger CI. If you need to trigger CI in the future, feel free to reach out to me here or Slack for quicker turnaround time. I'll trigger it now.

[christarazi]

test-me-please

[LexCC]

@jiejhih Only members with commit access can trigger CI. If you need to trigger CI in the future, feel free to reach out to me here or Slack for quicker turnaround time. I'll trigger it now.

Thanks! I just wonder why Cilium-Ginkgo-GKE unit test didn't passed, I'm not sure if that is related to this PR.

[christarazi]

@jiejhih Only members with commit access can trigger CI. If you need to trigger CI in the future, feel free to reach out to me here or Slack for quicker turnaround time. I'll trigger it now.

Thanks! I just wonder why Cilium-Ginkgo-GKE unit test didn't passed, I'm not sure if that is related to this PR.

We have known issues with tests running on GKE. I would consider this PR as passing.

[qmonnet]

Thanks!

[christarazi]

Looks like this PR had two identical commits that should have been squashed. We should be on the look out for issues like this next time before merging.

$ git log
...
* 8f0b57d0b kvstore: Add metric to count quorum errors
* a33605936 ci: refactor curl / wget test helpers with retries
* 21be15ebe ci: refactor curl / wget test helpers with retries
* 38d9bf589 bpf: explicitly set ttl in tunnel key

[pchaigno]

Marking for v1.8 backports to ease the backport of #13694.