Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Add tests for updateDependencies #1562

Merged
merged 3 commits into from
Dec 17, 2018

Conversation

mgazza
Copy link
Contributor

@mgazza mgazza commented Nov 29, 2018

Added tests to ensure that this functions behaviour with helm is always defined.

Added tests to ensure that this functions behaviour with helm is always defined.
@mgazza
Copy link
Contributor Author

mgazza commented Nov 29, 2018

Tests pass locally. I'm not sure how to proceed it looks like helm can't update the repository.

@squaremo
Copy link
Member

I suspect that it's because updateDependencies relies on the helm binary, and that's not available in the CI environment. The non-existent chart test succeeds there, because it expects an error, and gets one.

Suggestion: we download the helm client as part of the build, so it could just be a dependency of make test (and added to the path).

NB when I try this locally, it's the non-existent chart test that fails, i.e., it doesn't return an error. We can see if that's the case in CI ..

@squaremo
Copy link
Member

Update: added grin for first-time contributor -- thanks for this PR!

Some tests may depend on the helm binary, so make it a
dependency. And, since we want to test with the binary that's being
baked into the image, put it first on the $PATH.
Running locally, it's likely that you'll already have helm set up; but
this isn't the case in CI, and only a coincidence. Therefore, create a
temp directory and use that as Helm's home dir when testing.
@squaremo
Copy link
Member

@mgazza Are you happy for me to push a couple of commits here?

@mgazza
Copy link
Contributor Author

mgazza commented Dec 14, 2018

Hi @squaremo,
yes please! Sorry I meant to get back to you yesterday

@squaremo
Copy link
Member

@hiddeco Mind having a look at this one when you have a chance? I ought to recuse myself on the basis that I've added commits :-)

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @mgazza for your initial work on this 🥇

@squaremo squaremo merged commit 7f48703 into fluxcd:master Dec 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants