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

made AWS credentials optional #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

floriankick
Copy link

In case your docker host is a EC2 instance itself, the S3 access can be granted via IAM instance role. But this only works if AWS credentials are not passed directly to the CLI.

@@ -1,6 +1,6 @@
FROM ubuntu:18.04

RUN apt-get update && apt-get install -y --no-install-recommends curl cron awscli
RUN apt-get update && apt-get install -y --no-install-recommends curl cron awscli less vim
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need less and vim?

Copy link
Author

Choose a reason for hiding this comment

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

I suppose it was for debugging. Should probably be removed

@@ -8,7 +8,7 @@ cat <<EOF > env.sh
BACKUP_SOURCES="${BACKUP_SOURCES:-/backup}"
BACKUP_CRON_EXPRESSION="${BACKUP_CRON_EXPRESSION:-@daily}"
AWS_S3_BUCKET_NAME="${AWS_S3_BUCKET_NAME:-}"
BACKUP_FILENAME="$(date +"${BACKUP_FILENAME:-backup-%Y-%m-%dT%H-%M-%S.tar.gz}")"
BACKUP_FILENAME="\$(date +"${BACKUP_FILENAME:-backup-%Y-%m-%dT%H-%M-%S.tar.gz}")"
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't look intentional, either?

Comment on lines +25 to +31
if [ ! -z "$AWS_ACCESS_KEY_ID" ]; then
cat <<EOF > .aws/credentials
[default]
aws_access_key_id = ${AWS_ACCESS_KEY_ID}
aws_secret_access_key = ${AWS_SECRET_ACCESS_KEY}
EOF
fi
Copy link
Owner

Choose a reason for hiding this comment

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

This is missing some indentation..?

@fsimmend
Copy link

will this be merged some time in the future, identation cannot be a big issue.

@ojarva
Copy link

ojarva commented Mar 27, 2021

will this be merged some time in the future, identation cannot be a big issue.

Other problems like installing extra packages and adding incorrect escaping probably is, though.

jareware pushed a commit that referenced this pull request Dec 28, 2021
mpietruschka referenced this pull request in pietmacom/docker-volume-backup Mar 25, 2023
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.

5 participants