Bring helm integration test back

[tejal29]

Fixes #1823 Bring helm integration test back by adding namespace to release name

[codecov-io]

Codecov Report

Merging #2140 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2140   +/-   ##
=======================================
  Coverage   56.31%   56.31%           
=======================================
  Files         180      180           
  Lines        7815     7815           
=======================================
  Hits         4401     4401           
  Misses       2994     2994           
  Partials      420      420

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 010c517...032c613. Read the comment docs.

[nkubala]

do we want to set this here as opposed to just setting the namespace field?

[nkubala]

actually, this will break running the examples. if I don't have this field set:

Starting deploy...
Error: invalid release name, must match regex ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])+$ and the length must not longer than 53
Helm release skaffold-helm-<no value> not installed. Installing...
No requirements found in skaffold-helm/charts.
Error: release name skaffold-helm-<no value> is invalid: a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
WARN[0007] error retrieving helm deployment info: Error: invalid release name, must match regex ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])+$ and the length must not longer than 53

[corneliusweig]

@nkubala Are the "examples" below integration really meant to be run manually by users? I think it's fine to break these, as long as the actual user facing examples in examples do not need the TEST_NS env var.

[nkubala]

the examples in integration/ get synced to examples/ on every release:

skaffold/hack/release.sh

Lines 24 to 25 in 010c517

	# sync files from integration examples to examples/
	rm -rf ${EXAMPLES_DIR} && rm -rf ${INTEGRATION_EXAMPLES_DIR}/bazel/bazel-* && cp -r ${INTEGRATION_EXAMPLES_DIR} ${EXAMPLES_DIR} && rm -rf ${EXAMPLES_DIR}/test-*

so this will become a de facto example that we expect users to be able to run. I run these very frequently to test basic functionality so this will break my ability to do manual testing.

FYI the reason we have two directories and then sync them on release is so that when we merge something breaking to master, it doesn't break users trying to run their latest released version of skaffold on the examples/ directory.

[corneliusweig]

Ooops. I always thought the examples were synced in the other direction. But that makes perfect sense. (Thanks for the explanation, I would have asked..)

[tejal29]

hmm, i wasn't aware we sync examples from integration to examples
I think we should skip that for this change since you can't run this integration test in parallel.

[tejal29]

regarding

do we want to set this here as opposed to just setting the namespace field?

helm has this bug where release names not scoped by namespace. I think the ticket #1823 has more information.

[nkubala]

actually, this will break running the examples. if I don't have this field set:

Starting deploy...
Error: invalid release name, must match regex ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])+$ and the length must not longer than 53
Helm release skaffold-helm-<no value> not installed. Installing...
No requirements found in skaffold-helm/charts.
Error: release name skaffold-helm-<no value> is invalid: a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
WARN[0007] error retrieving helm deployment info: Error: invalid release name, must match regex ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])+$ and the length must not longer than 53

[corneliusweig]

@tejal29 Just for my understanding: is it correct that the reason why you add the namespace to the chart name is so that multiple integration tests can run in parallel?

[tejal29]

@corneliusweig

@tejal29 Just for my understanding: is it correct that the reason why you add the namespace to the chart name is so that multiple integration tests can run in parallel?

Yes, we need to add the namespace to chart name as helm release names are not namespace scoped. If 2 runs are running at the same time and 1 test run is in deleting the release "skaffold-helm" while other is creating it, helm ends up being in weird state.