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

Add option to publish via a different URI #70

Closed
wants to merge 2 commits into from

Conversation

steven-sheehy
Copy link

Fixes #45

This PR implements the proposal in PR #56, but addresses the concerns that I had with the proposal. It does so by maintaining the remote and local indexes separately such that the remote one has the published URI (HTTP) while the local always has the S3 URIs. Besides solving the problem with authenticated repositories while still supporting the other use cases, I think it is more intuitive that the Helm S3 plugin continues to use S3 as its protocol regardless of how the repository user consumes the publish repository.

I also merged the publish command into a helm s3 reindex --publish since the publish command was effectively a reindex command with a different publish URI.

cmd/helms3/delete.go Outdated Show resolved Hide resolved
cmd/helms3/push.go Outdated Show resolved Hide resolved
cmd/helms3/delete.go Outdated Show resolved Hide resolved
Signed-off-by: Steven Sheehy <ssheehy@firescope.com>
Signed-off-by: Steven Sheehy <ssheehy@firescope.com>
@steven-sheehy
Copy link
Author

I've added integration tests for published repositories. I also refactored the integration tests to remove the unnecessary if [ $? -ne 0 ] checks since -e will already catch non-zero return codes and to print out the failed test case.

@steven-sheehy
Copy link
Author

@hypnoglow Anything I can do to move this PR along?

@jgangemi
Copy link

any possible movement on this? i could also really use this functionality.

@steven-sheehy
Copy link
Author

steven-sheehy commented Feb 25, 2019

@jgangemi Until this is merged, you're welcome to use my forked build:

helm plugin install https://github.com/steven-sheehy/helm-s3.git --version v0.9.0

@jgangemi
Copy link

@steven-sheehy awesome, thanks!

what's the behavior if i still want to access via s3? will both protocols work to read the charts or am i locked into one or the other.

@steven-sheehy
Copy link
Author

@jgangemi Nope, you can use both concurrently, even from the same client. Just add the s3 and http as separate repos via helm repo add. The s3 repo can be used for read/write while the http can do reads.

@jgangemi
Copy link

@steven-sheehy awesome, thanks again!!

@jgangemi
Copy link

jgangemi commented Mar 1, 2019

hey @steven-sheehy, you didn't touch anything w/ the push that would have affected * being resolved, eg:

helm s3 push --dry-run target/* repo

this used to work in an older version and doesn't ask of 0.9.0

@steven-sheehy
Copy link
Author

My changes shouldn't affect the issue you're experiencing. * is expanded by bash not the s3 plugin. But helm s3 push [<flags>] <chartPath> <repo> doesn't take multiple charts paths so if bash happened to expand to multiple paths it wouldn't work.

@jgangemi
Copy link

jgangemi commented Mar 1, 2019

weird b/c this used to work and helm lint charts/* earlier in the script has no issues. i'm sure i can change the script figure out the dirname.

thanks for responding and also doing this work, it saved me from having to figure out a hacky solution.

@rverma-nikiai
Copy link

@hypnoglow can we merge this pr

@jvosantos
Copy link

@hypnoglow is this going to be merged? it's been some months now

@cplee
Copy link

cplee commented Mar 13, 2020

@steven-sheehy - do you have your fork hosted anywhere that I can move over to? This PR looks abandoned...

@davidkarlsen
Copy link

Can this or #56 be merged? I use the plugin to publish the charts to a s3 bucket, which is then hosted by a cloudfront distro, hence I had to hack the index.yaml:

     urls:
-    - s3://thebucketname/fspython-0.1.0.tgz
+    - fspython-0.1.0.tgz

@steven-sheehy
Copy link
Author

I don't want to spend time fixing the conflicts (which are not trivial) unless there's some indication the PR has the possibility of being reviewed and accepted.

@davidkarlsen
Copy link

@hypnoglow care to comment?

@davidkarlsen
Copy link

Also - I think this can be simplified, by having the list in the url element only list the archive name - i.e. without the protocol&host prefix - that worked for me - and they are resolved relative to the repo given.

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.

Allow anonymous access or base url in index to be overridden
7 participants