-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
helm chart for transfer.sh #255
base: master
Are you sure you want to change the base?
Conversation
hi @beyondszine thanks for the PR, it is really welcome anyway I cannot merge as it is with default S3 provider: we should provide proper switch in the values.yaml to generate the template for any provider type (local, s3 and gdrive) let me know if you have time to do it otherwise I will do as soon I'll have time based on your branch thanks |
Dockerfile
Outdated
@@ -10,6 +10,7 @@ ADD . /go/src/github.com/dutchcoders/transfer.sh | |||
WORKDIR /go/src/github.com/dutchcoders/transfer.sh | |||
|
|||
ENV GO111MODULE=on | |||
ENV APP_PORT=80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will work for new helm template but actually break everything else, please, keep 8080
as listener port
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, indeed. Its my mistake. I forgot about it while pushing. Will make sure it stays same as per transfersh's convention.
For sure. It makes sense. Also, I want to ask your opinion on a few things.
I asked above points because this way, may be a very general helm template can be made to inject param values(if they follow some standard) from values.yaml file. -- |
Added support for both. values.yaml file contains .Values.argValues.source.type can be secretKeyRef or configMapKeyRef
Added as there comes some issues for hyphens in some shells with env vars.
All the values in given by user at .Values.argValues.paramNames in values.yaml will be fetched, converted to secret/configMap name & injected in container at runtime. Let me know your views on above points.
|
@beyondszine thank you for your effort. I will try to review and give proper feedback by the end of the week |
Sure thing. |
"type" : "secretKeyRef", | ||
"name" : "transfersh-secrets" | ||
}, | ||
"paramNames" :[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot of parameters missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
umm, I couldn't get what you mean here.
The idea why I wrote this is,
whatever params are put in this array of 'paramNames' will be reflected into the secrets automatically(because of the loop). I only put a few for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then either this is a POC or it only supports S3 provider
for me in order for the branch to be merged should support every provider/flag that transfer.sh supports
kubectl run transfersh --restart=Never --image=dutchcoders/transfer.sh:latest -- --http-auth-user my-username --http-auth-pass somepassword --provider s3 --aws-access-key $AWS_ACCESS_KEY_ID --aws-secret-key $AWS_SECRET_ACCESS_KEY --bucket $AWS_TRANSFERSH_BUCKET --s3-region $AWS_TRANSFERSH_BUCKET_REGION | ||
|
||
# Example to manually create needed secrets for deployment params totally aligned with [Usage Params](https://github.com/dutchcoders/transfer.sh#usage-1) | ||
kubectl create secret generic transfersh-secrets --from-literal=HTTP_AUTH_USER=$HTTP_AUTH_USER --from-literal=HTTP_AUTH_PASS=$HTTP_AUTH_PASS --from-literal=AWS_ACCESS_KEY=$AWS_ACCESS_KEY --from-literal=AWS_SECRET_KEY=$AWS_SECRET_KEY --from-literal=BUCKET=$BUCKET --from-literal=S3_REGION=$S3_REGION --from-literal=PROXY_PATH=$PROXY_PATH --from-literal=PROVIDER=$PROVIDER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed I don't need a secret is needed, but just a config map.
anyway, that's not the problem: any solution we chose should be taken care of by the helm template, not from some previous setup directly in k8s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
umm, this is just for an example of how a secret addition can be done as AWS_ACCESS & AWS_SECRET are things people may not want to put in configmap. But anyway, the option to put both configMap/secret is given by the values.yaml.
Addition of params & their values in configmap can also be given inthe readme.
Idea of putting it in REAMDE was to enable people to also be able to manually do deployment,service,secret/configmap in their cluster Other than automated helm-chart deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem is not being a secret or a config map
the problem is that in order to use the helm chart I have to create the secret directly in k8s
this should be taken care of by the chart itself
name: {{ $keyRefName }} | ||
key: {{ . | upper | replace "-" "_"}} | ||
{{- end }} | ||
args: [ {{- range .Values.argValues.paramNames }} {{ printf "%s%s" $.Values.argIdentifier . | quote}},{{ printf "%s%s%s" "$(" . ")" | upper | replace "-" "_" | quote}},{{- end }} ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the moment, assuming we will add the missing supported params, it should work, because currently no param is mutually exclusive to each other
tomorrow this would not be true, anyway: we may have conflicting params
also, even if I've not tested, for bool param having the flag set could be enough to enable them: so passing them by default should be avoided, and rather passed or not according to their value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess I understand what you mean.
But isn't the job of params validation & their compatibility should be taken care by the binary itself and not the wrapping frameworks(docker,CI/CD,kubernetes etc.).
If the params have conflict, shouldn't they be just passed through from outside to the binary and let binary handle them & give appropriate error.
For bool values, you point makes sense. But since our param names are not indicating if they are bool or not, then there must be a way in helm layer to know the difference between a bool param & otherwise.
May be you can give me some hint on some you would want Values.yaml to be(specifically declaring params), so that I can backtrack & understand better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the binary does params validation: if you pass --provider local
without --basedir
it won't start
tomorrow it could be the case that if you pass params related to S3 when using local provider will fail validation as well: if the chart just passes everything it won't work or will enable functionality that use doesn't want to enable (like with the bool flags)
I'm not such an expert of helm so I don't know what's the most idiomatic way to solve this issue: I guess having different values.yaml
for every different provider can be a starting point. for bool flags we could use flag.enabled
like you did for the ingress and then pass --set flag.enabled=true
to helm
people that has a stable/immutable deployment needing can write their own custom values.yaml
, others can rely on the default provided ones and the --set
flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get it now, what you mean here & above.
Give me some time, let me get back to you on how I think what you say, can be implemented (only as a plan) & if we agree, then probably I can fix changes in code.
Hi there,
This pull request tries to add helm chart feature for transfer.sh. I have currently deployed this chart in my testing cluster. Let me know in case of any changes required.