Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove filename restrictions #120

Merged
merged 13 commits into from
Nov 13, 2020
Merged

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Oct 29, 2020

Fixes: #119

@jkroepke
Copy link
Contributor Author

jkroepke commented Nov 1, 2020

I'm using my branch in my CI. It works fine.

@aslafy-z
Copy link
Owner

aslafy-z commented Nov 1, 2020

Looks good! Can you add at least one test case?

@jkroepke
Copy link
Contributor Author

jkroepke commented Nov 1, 2020

Can you add at least one test case?

Done.

Take a look at:

https://github.com/bats-core/bats-assert
https://github.com/bats-core/bats-file

It should improve the readability of the test files.

Take a look here: https://github.com/jkroepke/helm-secrets/blob/master/tests/unit/clean.bats

@jBouyoud
Copy link
Contributor

jBouyoud commented Nov 9, 2020

@aslafy-z Does it ready to merge and release ?

@jkroepke
Copy link
Contributor Author

jkroepke commented Nov 9, 2020

@jBouyoud you could take a look why tests are failed.

@jBouyoud
Copy link
Contributor

@jkroepke --repo flag is not supported on helm 2.

Maybe you can move your test to 03-cli.bats and use something like :

@test "fetch tests/fixtures/example-chart/values.yaml" {
    run helm_init "$HELM_HOME"
    url="git+https://github.com/aslafy-z/helm-git@tests/fixtures/example-chart/values.yaml?ref=master"
    $HELM_GIT_DIRNAME/helm-git "" "" "" "$url" 2>/dev/null > "$HELM_GIT_OUTPUT/values.yaml"
    [ $? = 0 ]
    run stat "$HELM_GIT_OUTPUT/values.yaml"
    [ $status = 0 ]
}

@jkroepke
Copy link
Contributor Author

flag is not supported on helm 2.

The whole feature does not exist in Helm 2.

@jBouyoud
Copy link
Contributor

@jkroepke I don't understand your comment about Helm 2.
Based on https://github.com/aslafy-z/helm-git/blob/master/helm-git-plugin.sh#L96, this plugin seems design for Helm 2 and hopefully works on Helm3.

This commit jBouyoud@345cc81 add a test for this feature and remove the non compatible one. You can cherry pick it, I cannot push to your branch.

@aslafy-z
Copy link
Owner

aslafy-z commented Nov 12, 2020

Thanks @jkroepke @jBouyoud for your work. You're right, this plugin was designed at the time of Helm 2 and some awesome contributors worked on it to add support for Helm 3. I want to make it Helm 3 first in the future, will see when I get time to work on it.

@jkroepke can you merge in @jBouyoud change so we can push this forward? If you have time, adding a quick example to the readme would be perfect!

@jkroepke thanks for the tests sample, I opened #127 to track this enhancement. helm-secrets looks good!

@jkroepke
Copy link
Contributor Author

@jkroepke I don't understand your comment about Helm 2.

I thought, the download plugins are helm 3 exclusive feature. But this is wrong.

@jkroepke
Copy link
Contributor Author

jkroepke commented Nov 12, 2020

@jkroepke can you merge in @jBouyoud change so we can push this forward?

Done.

@jkroepke
Copy link
Contributor Author

jkroepke commented Nov 12, 2020

Test are green, but tests are not executed.

See: #128

@aslafy-z aslafy-z changed the title Remove filename restrictions feat: remove filename restrictions Nov 13, 2020
@aslafy-z aslafy-z merged commit d1388b1 into aslafy-z:master Nov 13, 2020
@aslafy-z
Copy link
Owner

Released as v0.10.0. Big thanks!

@jkroepke jkroepke deleted the patch-1 branch November 13, 2020 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Target file name has to be either 'index.yaml' or a tgz release
3 participants