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.

Sign up for GitHub

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

Jump to bottom

Allow the S3 bucket name to be specified via AWS_S3_BUCKET #5376

Merged
merged 1 commit into from
Mar 13, 2015
Merged

Allow the S3 bucket name to be specified via AWS_S3_BUCKET #5376

merged 1 commit into from
Mar 13, 2015

Conversation

justinsb
Copy link
Member

No description provided.

@a-robinson a-robinson self-assigned this Mar 12, 2015
@a-robinson
Copy link
Contributor

LGTM, but

  1. How would a new user be expected to know that this is available as an option? Would it hurt to also add it to the list in config-default.sh?
  2. You'll have to rebase this on top of Don't make the s3 bucket world-readable (just the files) #5377 to make it mergable

@justinsb justinsb force-pushed the aws_customize_s3_bucket branch from 1d9823c to bb622cc Compare March 12, 2015 17:26
@justinsb
Copy link
Member Author

Good point. I think we should probably have an options.md file or something like that, but for now I've taken your suggestion and put it into config-default.sh

We don't want to actually set a default (non-empty) value though, because we want to generate a (pseudo) unique name unless the user takes responsibility for picking the bucket name (because bucket names are global across regions and users).

@justinsb justinsb mentioned this pull request Mar 12, 2015
Merged
@justinsb justinsb force-pushed the aws_customize_s3_bucket branch from bb622cc to 26cf662 Compare March 12, 2015 18:08
@justinsb
Copy link
Member Author

I fixed the merge-conflict here (not a real conflict, I had just changed adjacent lines).

a-robinson
a-robinson reviewed Mar 12, 2015
View reviewed changes
cluster/aws/util.sh
fi

local -r staging_path="${staging_bucket}/devel"
aws s3api put-bucket-acl --bucket ${AWS_S3_BUCKET} --acl public-read
Copy link
Contributor

Choose a reason for hiding this comment

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

This would undo #5377

@justinsb
Copy link
Member Author

Wow... I'm not doing very well here. Sorry & nice catch. Let me fix this one.

I'm going to work on one patch at a time. Starting with this one!

@justinsb justinsb force-pushed the aws_customize_s3_bucket branch from 26cf662 to 3f71cc9 Compare March 12, 2015 18:33
@a-robinson
Copy link
Contributor

Ah, sorry about the delay here. I looked at it right after I got an email about your comment only to see that it still wasn't mergable. Looking again now.

a-robinson
a-robinson reviewed Mar 13, 2015
View reviewed changes
cluster/aws/util.sh
else
project_hash=$(echo -n "${USER} ${key}" | md5sum | awk '{ print $1 }')
fi
AWS_S3_BUCKET="kubernetes-staging-${project_hash}/"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you want the slash on the end of the bucket name here, do you?

@justinsb justinsb force-pushed the aws_customize_s3_bucket branch from 3f71cc9 to e3e6c83 Compare March 13, 2015 20:44
@justinsb
Copy link
Member Author

Thanks for the catch (again). I removed the extra trailing slash!

a-robinson added a commit that referenced this pull request Mar 13, 2015
@a-robinson
Merge pull request #5376 from justinsb/aws_customize_s3_bucket
f6441a6 
Allow the S3 bucket name to be specified via AWS_S3_BUCKET
@a-robinson a-robinson merged commit f6441a6 into kubernetes:master Mar 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@a-robinson a-robinson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@justinsb @a-robinson @googlebot