Add tests for helm completion (#6115)

[marckhouzam]

What this PR does / why we need it:

This commit adds a framework and tests for helm command-line
completion.

To cover different shells (bash 4, bash 3, zsh) we use docker.

For MacOS, the tests are run locally if the host is MacOS and has
the right setup (bash completion installed, or zsh installed).

Dynamic-completion (e.g., helm status <TAB>) is not yet tested, but has
been considered in the design of the framework.

To run the tests:
make test-completion

Although the framework supports marking tests as known failures,
this commit commented out the failing tests to allow for a clean
output for this proposal. They can be uncommented later on.

A new directory is added: scripts/completion-tests.
It contains the required scripts for the completion tests.

scripts/completion-tests/completionTests.sh:

• helm-specific completion tests.
• This script should evolve as new tests are added and new completion features implemented.

scripts/completion-tests/test-completion.sh:

• This script repeatedly runs the completion-tests in different environments using docker.
• This script would need changes if a new environment needs to be added.

scripts/completion-tests/lib/completionTests-base.sh:

• The magic. It is this script that allows completion testing.
• This script is not expected to change in day-to-day helm evolution.

closes #6115

Special notes for your reviewer:

To see a test fail, you can uncomment one of the failing tests of
scripts/completion-tests/completionTests.sh
or you can modify the expected result of one of those tests to something bogus such as
_completionTests_verifyCompletion "helm stat" "statusBAD"

Interesting example is that the test
#_completionTests_verifyCompletion ZFAIL "helm --kubeconfig=/tmp/config lis" "list"
can be seen to start working if we apply PR #5680. Should allow to review that PR with more confidence.

[mattfarina]

Thanks for the PR.

@jdolitsky recently started an acceptance testing repo. I wonder if this is the type of thing that belongs in there or here.

[mattfarina]

Any chance these tests could be made to work on multiple platforms?

[marckhouzam]

I made the assumption that the person running these tests would run docker under linux/amd64. I can try to figure out where docker is being run and choose the correct platform.

Is that what you mean?

[mattfarina]

The copyright should read:

Copyright The Helm Authors.

Can you please update it throughout the PR.

[marckhouzam]

I was hoping you'd catch this since I wasn't sure how to handle it.

IIUC the DCO is a way for me to contribute this code to the project under the licence used by the project; the DCO does not transfer the copyright ownership of the code however.

So, since I'm not a Helm Author and don't have authorization to give away the copyright, I didn't think it was ok to put the "The Helm Authors". In other projects I've seen the form <original copyright holder> and others to more appropriately reflect the copyright ownership.

Do you know of guidelines from the CNCF on this?

I don't mean to be a pain, but I also don't want to overstep what I'm allowed to do.

[bacongobbler]

IIRC the CNCF wants all projects to be Apache 2 licensed. One of the reasons is because of point 5 under the terms of use, which calls this out explicitly: https://github.com/helm/helm/blob/master/LICENSE#L131-L137

If this code is copyrighted, then the CNCF team has made one recommendation before: if a contributor is providing code from another project, it should be stored in a directory that is explicitly known to come from a third party, and the licenses can be stored there under a NOTICE. Kubernetes does this with a /third-party directory in their codebase, so that may be worth looking into.

[marckhouzam]

This contribution is made under the Apache 2 license, so it can be part of the project directly.

It's just that the Copyright ownership is not transferred by the DCO, so I thought it should be written properly who the copyright owner is, in this case Ville de Montreal. But the license remains Apache 2.

[marckhouzam]

To complete on this discussion, I've contacted the CNCF about the copyright question and they explained that by making a contribution to the Helm project, my employer (through me) becomes a "Helm Author". Therefore, it is ok to put "Copyright The Helm Authors" as part of my contribution even though the copyright is not transferred. Sorry for me not realizing this.

[marckhouzam]

@mattfarina Good point about the acceptance-testing repo. I hesitated after seeing the announcement about this new repo. However, I liked the fact that these tests could be run while doing development as it would allow to handle regressions of the completion logic more quickly.

When/if we move forward and get to the point of adding testing of dynamic completion, I was going to propose to avoid using a real kubernetes cluster and instead use a fake store as is done with the go tests. So the completion tests could still be run easily by the developer.

I'm just voicing my opinion but of course it is your call.

[jdolitsky]

@marckhouzam originally I had PR'd the acceptance tests to helm/helm under the same premise that I would like to run them during development. However, it is easy enough to just run a "make build" and set PATH appropriately prior to running the acceptance test suite.

On that basis, I'd be in favor of moving this stuff over to helm/acceptance-testing, under some new file completion.robot. Let me know if you want to chat about this.

Criteria for what to put in acceptance test repo should probably be "does a fully-built version of Helm work in its target environment(s)?" (different Kubernetes clusters, different shells, etc.)

[marckhouzam]

@jdolitsky Thanks for the context. I'll have a deeper look at the acceptance-testing repo to see how things could work there. I'll post back once I make progress or if I have questions :-)

[marckhouzam]

I'm closing this PR as it was moved to the acceptance-testing repo:
helm/acceptance-testing#12

The question of Copyright is still under discussion.