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

Allow to have custom bucket name and region #1265

Merged
merged 1 commit into from Jan 28, 2016

Conversation

Projects
None yet
6 participants
@talset
Copy link
Contributor

talset commented Jan 22, 2016

File playbooks/adhoc/s3_registry/s3_registry*

To be able to use a different bucket name and region, aws_bucket and aws_region are now available

  • Add variable for region and bucket into j2
  • Update comment Usage
  • Add default aws_bucket_name and aws_bucket_region
@openshift-bot

This comment has been minimized.

Copy link
Collaborator

openshift-bot commented Jan 22, 2016

Can one of the admins verify this patch?

@rh-atomic-bot

This comment has been minimized.

Copy link

rh-atomic-bot commented Jan 22, 2016

Can one of the admins verify this patch?
I understand the following comments:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once
@@ -13,6 +13,8 @@
vars:
aws_access_key: "{{ lookup('env', 'S3_ACCESS_KEY_ID') }}"
aws_secret_key: "{{ lookup('env', 'S3_SECRET_ACCESS_KEY') }}"
aws_bucket_name: "{{ aws_bucket | default(clusterid + '-docker') }}"
aws_bucket_region: "{{ aws_region | default('eu-west-1') }}"

This comment has been minimized.

@detiber

detiber Jan 22, 2016

Contributor

Could you set the default to 'us-east-1' as it was the previous default?

@@ -13,6 +13,8 @@
vars:
aws_access_key: "{{ lookup('env', 'S3_ACCESS_KEY_ID') }}"
aws_secret_key: "{{ lookup('env', 'S3_SECRET_ACCESS_KEY') }}"
aws_bucket_name: "{{ aws_bucket | default(clusterid + '-docker') }}"

This comment has been minimized.

@detiber

detiber Jan 22, 2016

Contributor

http://jinja.pocoo.org/docs/dev/templates/ recommends using '~' instead of '+' for string concatenation, so:

aws_bucket_name: "{{ aws_bucket | default(clusterid + '-docker') }}"

should be:

aws_bucket_name: "{{ aws_bucket | default(clusterid ~ '-docker') }}"
@talset

This comment has been minimized.

Copy link
Contributor

talset commented Jan 22, 2016

Thanks @detiber , I just fixed and push

@detiber

This comment has been minimized.

Copy link
Contributor

detiber commented Jan 22, 2016

ok to test

@detiber

This comment has been minimized.

Copy link
Contributor

detiber commented Jan 22, 2016

@dak1n1 ptal

@@ -13,6 +13,8 @@
vars:
aws_access_key: "{{ lookup('env', 'S3_ACCESS_KEY_ID') }}"
aws_secret_key: "{{ lookup('env', 'S3_SECRET_ACCESS_KEY') }}"
aws_bucket_name: "{{ aws_bucket | default(clusterid ~ '-docker') }}"
aws_bucket_region: "{{ aws_region | default('us-east-1') }}"

This comment has been minimized.

@dak1n1

dak1n1 Jan 27, 2016

Contributor

Looks good. We might also want to add a lookup to check and see if the region is already specified in an env variable.

aws_bucket_region: "{{ aws_region | lookup('env', 'EC2_REGION') | default('us-east-1') }}"

@talset

This comment has been minimized.

Copy link
Contributor

talset commented Jan 28, 2016

Ok done, I added
aws_bucket_region: "{{ aws_region | lookup('env', 'EC2_REGION') | default('us-east-1') }}"

But maybe it should be S3_REGION instead of EC2_REGION ?

@@ -13,6 +13,8 @@
vars:
aws_access_key: "{{ lookup('env', 'S3_ACCESS_KEY_ID') }}"
aws_secret_key: "{{ lookup('env', 'S3_SECRET_ACCESS_KEY') }}"
aws_bucket_name: "{{ aws_bucket | default(clusterid ~ '-docker') }}"
aws_bucket_region: "{{ aws_region | lookup('env', 'EC2_REGION') | default('us-east-1') }}"

This comment has been minimized.

@detiber

detiber Jan 28, 2016

Contributor

@talset I agree on this being S3_REGION instead of EC2_REGION

Allow to have custom bucket name and region
File playbooks/adhoc/s3_registry/s3_registry*

To be able to use a different bucket name and region, aws_bucket and aws_region are now available

  * Add variable for region and bucket into j2
  * Update comment Usage
  * Add default aws_bucket_name and aws_bucket_region
@talset

This comment has been minimized.

Copy link
Contributor

talset commented Jan 28, 2016

Done ;)

@detiber

This comment has been minimized.

Copy link
Contributor

detiber commented Jan 28, 2016

lgtm @twiest

@twiest

This comment has been minimized.

Copy link
Contributor

twiest commented Jan 28, 2016

@dak1n1 Can you please review this? if it looks good, please give it a +1 and merge it.

Thanks.

@dak1n1

This comment has been minimized.

Copy link
Contributor

dak1n1 commented Jan 28, 2016

👍

dak1n1 added a commit that referenced this pull request Jan 28, 2016

Merge pull request #1265 from talset/s3_registry_clusterid
Allow to have custom bucket name and region

@dak1n1 dak1n1 merged commit d92f00d into openshift:master Jan 28, 2016

1 check passed

default No test results found.
Details
@talset

This comment has been minimized.

Copy link
Contributor

talset commented Jan 29, 2016

@dak1n1 I Tried your "{{ aws_region | lookup('env', 'EC2_REGION') | default('us-east-1') }}" syntax and I have a warning. In fact the default() function is required.
So I create a PR [1] if you could have a look ?

[1] #1303

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment