Custom s3 endpoint support #28

pitscher · 2020-05-27T11:47:30Z

As mentioned in #27 this is the PR for supporting custom s3 endpoints.
I successfully ran the e2e script for AWS (because noting GCP related was changed).

If you don't set the new env var AWSCLI_ENDPOINT_URL_OPTION the default s3 endpoint will be used.
If you set the var, the provided endpoint will be used. That's it. No rocket science. 😄

BTW: I updated the README as well.

ryu-sato

Thank you very much for your contributions.

Could you use the word "S3" instead of "s3" in README.md?

functions.sh LGTM.

skomma · 2020-05-28T02:18:16Z

bin/functions.sh

@@ -19,7 +20,7 @@ DATE_CMD="/bin/date"
 # arguments: 1. s3 url (s3://.../...)
 s3_exists() {
 	if [ $# -ne 1 ]; then return 255; fi
-	${AWSCLI} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1 >/dev/null
+	${AWSCLI} ${AWSCLI_ENDPOINT_URL_OPTION} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1 >/dev/null


AWSCLI_ENDPOINT_URL_OPTION will be set like fra1.digitaloceanspaces.com, so --endpoint-url is needed here, right?

Yes. It's important to set the whole aws cli parameter "--endpoint-url https://domain.tld".
It is easier to implement this way because if a user wanna use the default S3 endpoint the only thing he must ensure is to not set the AWSCLI_ENDPOINT_OPT var. That's it. 😃

skomma · 2020-05-28T02:22:39Z

bin/functions.sh

@@ -1,5 +1,6 @@
 # default settings
 AWSCLI="/usr/bin/aws"
+#AWSCLI_ENDPOINT_URL_OPTION="--endpoint-url <YourEndpointUrl>"
 AWSCLI_COPY_OPT="s3 cp"
 AWSCLI_LIST_OPT="s3 ls"
 AWSCLI_DEL_OPT="s3 rm"


Since AWSCLIOPT is already provided, I think you can do the same thing by using this environment variable without adding AWSCLI_ENDPOINT_URL_OPTION.

Unfortunately, currently AWSCLIOPT is not documented...

To provide the ability to use the default endpoint or a custom one and not over engineer the code we should use a new env var. Therefore I introduced AWSCLI_ENDPOINT_OPT

pitscher · 2020-05-28T14:41:00Z

As mentioned here: #28 (comment)
I updated a few things like the name of the new ENDPOINT env var and renamed "s3" to "S3".
Now all should be fine. 🤓

pitscher · 2020-06-01T23:54:45Z

Hi guys,
could you give me an update regarding my last changes? 😃

ryu-sato

Comparing the usage and implementation of AWSCLI_ENDPOINT_OPT, README.md lists "fra1.digitaloceanspaces.com" in the input example, but functions.sh expects user to specify "--endpoint-url" as in "--endpoint-url fra1.digitaloceanspaces.com", so you need to match two.

How to modify it...

Change the input example of AWSCLI_ENDPOINT_OPT to "--endpoint-url fra1.digitaloceanspaces.com".
In functions.sh, if AWSCLI_ENDPOINT_OPT exists, pass "--endpoint-url $AWSCLI_ENDPOINT_OPT" to aws options.
Delete AWSCLI_ENDPOINT_OPT from README.md and functions.sh, and describe "--endpoint-url fra1.digitaloceanspaces.com" as an example of AWSCLIOPT input.

1 is unacceptable because the name AWSCLI_ENDPOINT_OPT is different from the actual behavior because it allows user to specify an AWS option other than "--endpoint-url".
2 is closer to the current implementation than 3, so can you modify it with this policy?

Also, please modify the following points in the README (If you chose policy 2)

Make the input example of AWSCLI_ENDPOINT_OPT into a URL
- ex. "AWSCLI_ENDPOINT_OPT="https://fra1.digitaloceanspaces.com"
Add AWSCLI_ENDPOINT_OPT to the three docker run command execution examples in Usage
- ex. [ -e AWSCLI_ENDPOINT_OPT=<S3 endpoint URL (ex. https://fra1.digitaloceanspaces.com)> \ ]

pitscher · 2020-06-02T20:27:07Z

Thank you for the feedback 😃
I've implemented what you requested. Hopefully this correspond to your 2. policy.

ryu-sato

We want the code to be DRY.

It is better not to use if many times according to the value of AWSCLI_ENDPOINT_OPT.
If you set it to a variable beforehand, you don't have to repeat using if.

In addition to the suggestions I commented on, please add the following to line 6 of "bin/functions.sh"

AWSCLI_ENDPOINT_OPT=${AWSCLI_ENDPOINT_OPT:+"--endpoint-url ${AWSCLI_ENDPOINT_OPT}"}

ryu-sato · 2020-06-03T16:43:25Z

bin/functions.sh

+	  if [ -z "$AWSCLI_ENDPOINT_OPT" ]
+		then
+			${AWSCLI} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1 >/dev/null
+		else
+			${AWSCLI} --endpoint-url ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1 >/dev/null
+		fi


Suggested change

if [ -z "$AWSCLI_ENDPOINT_OPT" ]

then

${AWSCLI} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1 >/dev/null

else

${AWSCLI} --endpoint-url ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1 >/dev/null

fi

${AWSCLI} ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1 >/dev/null

ryu-sato · 2020-06-03T16:47:26Z

bin/functions.sh

+	if [ -z "$AWSCLI_ENDPOINT_OPT" ]
+	then
+		${AWSCLI} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1
+	else
+		${AWSCLI} --endpoint-url ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1
+	fi


Suggested change

if [ -z "$AWSCLI_ENDPOINT_OPT" ]

then

${AWSCLI} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1

else

${AWSCLI} --endpoint-url ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1

fi

${AWSCLI} ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_LIST_OPT} $1

ryu-sato · 2020-06-03T16:47:52Z

bin/functions.sh

+		if [ -z "$AWSCLI_ENDPOINT_OPT" ]
+		then
+			${AWSCLI} ${AWSCLIOPT} ${AWSCLI_DEL_OPT} $1
+		else
+			${AWSCLI} --endpoint-url ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_DEL_OPT} $1
+		fi


Suggested change

if [ -z "$AWSCLI_ENDPOINT_OPT" ]

then

${AWSCLI} ${AWSCLIOPT} ${AWSCLI_DEL_OPT} $1

else

${AWSCLI} --endpoint-url ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_DEL_OPT} $1

fi

${AWSCLI} ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_DEL_OPT} $1

ryu-sato · 2020-06-03T16:50:21Z

bin/functions.sh

+		if [ -z "$AWSCLI_ENDPOINT_OPT" ]
+		then
+			${AWSCLI} ${AWSCLIOPT} ${AWSCLI_COPY_OPT} $1 $2
+		else
+			${AWSCLI} --endpoint-url ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_COPY_OPT} $1 $2


There is no "fi" on line 80 and it is a syntax error.

Suggested change

if [ -z "$AWSCLI_ENDPOINT_OPT" ]

then

${AWSCLI} ${AWSCLIOPT} ${AWSCLI_COPY_OPT} $1 $2

else

${AWSCLI} --endpoint-url ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_COPY_OPT} $1 $2

${AWSCLI} ${AWSCLI_ENDPOINT_OPT} ${AWSCLIOPT} ${AWSCLI_COPY_OPT} $1 $2

pitscher · 2020-06-03T19:04:27Z

Of course. Good point! This should finally be what you expected.
Thank you for this sleek tip. I mean... I created the PR and you invested a bunch of your time so far.
But I appreciate your help. I learned a lot during this PR. 😄

ryu-sato

LGTM

pitscher and others added 3 commits May 26, 2020 14:49

Added new env var for custom endpoint

99bce93

Removed otc endpoint url and added placeholder

feeee33

Include custom s3 endpoint option in README

55c1e2a

ryu-sato reviewed May 28, 2020

View reviewed changes

skomma requested changes May 28, 2020

View reviewed changes

pitscher added 2 commits May 28, 2020 16:27

Rename ENDPOINT env var

f6d75c5

Update README with new renamed ENDPOINT var

05c7fa8

pitscher requested a review from skomma May 28, 2020 14:41

ryu-sato requested changes Jun 2, 2020

View reviewed changes

pitscher added 3 commits June 2, 2020 21:17

Remove old comment

571a317

Add check if custom aws endpoint is set

52eccb5

Add custom endpoint option to docker run examples

8e1f685

pitscher requested a review from ryu-sato June 2, 2020 20:36

ryu-sato requested changes Jun 3, 2020

View reviewed changes

Remove repetitive if-statements

4b4c8a2

pitscher requested a review from ryu-sato June 3, 2020 19:04

ryu-sato approved these changes Jun 4, 2020

View reviewed changes

ryu-sato merged commit d7e690f into weseek:master Jun 4, 2020

pitscher deleted the custom-s3-endpoint-support branch June 4, 2020 16:53

ryu-sato linked an issue Jun 4, 2020 that may be closed by this pull request

Support for custom s3 endpoints #27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom s3 endpoint support #28

Custom s3 endpoint support #28

pitscher commented May 27, 2020

ryu-sato left a comment

skomma May 28, 2020

pitscher May 28, 2020 •

edited

Loading

skomma May 28, 2020

pitscher May 28, 2020 •

edited

Loading

pitscher commented May 28, 2020

pitscher commented Jun 1, 2020

ryu-sato left a comment

pitscher commented Jun 2, 2020

ryu-sato left a comment

ryu-sato Jun 3, 2020

ryu-sato Jun 3, 2020

ryu-sato Jun 3, 2020

ryu-sato Jun 3, 2020

pitscher commented Jun 3, 2020

ryu-sato left a comment

Custom s3 endpoint support #28

Custom s3 endpoint support #28

Conversation

pitscher commented May 27, 2020

ryu-sato left a comment

Choose a reason for hiding this comment

skomma May 28, 2020

Choose a reason for hiding this comment

pitscher May 28, 2020 • edited Loading

Choose a reason for hiding this comment

skomma May 28, 2020

Choose a reason for hiding this comment

pitscher May 28, 2020 • edited Loading

Choose a reason for hiding this comment

pitscher commented May 28, 2020

pitscher commented Jun 1, 2020

ryu-sato left a comment

Choose a reason for hiding this comment

pitscher commented Jun 2, 2020

ryu-sato left a comment

Choose a reason for hiding this comment

ryu-sato Jun 3, 2020

Choose a reason for hiding this comment

ryu-sato Jun 3, 2020

Choose a reason for hiding this comment

ryu-sato Jun 3, 2020

Choose a reason for hiding this comment

ryu-sato Jun 3, 2020

Choose a reason for hiding this comment

pitscher commented Jun 3, 2020

ryu-sato left a comment

Choose a reason for hiding this comment

pitscher May 28, 2020 •

edited

Loading

pitscher May 28, 2020 •

edited

Loading