Ft/more s3 locations support #1207

alexanderchan-scality · 2018-04-13T00:04:47Z

Adds more support options for S3 location constraints:

optional versioning for S3 backend
path style

ironman-machine · 2018-04-13T16:51:30Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-13T16:54:28Z

PR has been updated. Reviewers, please be cautious.

philipyoo

lgtm, but may need to consider using a different whatwg url lib

philipyoo · 2018-04-13T17:37:36Z

lib/management.js

@@ -1,3 +1,4 @@
+const { URL } = require('url');


I don't think url.URL is supported in Node v6. Instead there is a property: url.Url but it doesn't seem to work as intended.

Based on this blog: https://www.contentful.com/blog/2016/11/02/node-v7-urls-deprecations-warnings-and-developer-experience/
Looks like support was added in v7?

https://nodejs.org/docs/latest-v6.x/api/url.html#url_the_whatwg_url_api
it is also added in 6.13

jonathan-gramain

one style comment, but it looks good to me

jonathan-gramain · 2018-04-13T22:21:57Z

lib/management.js

                    break;
-                case 'location-do-spaces-v1':
+                case 'location-scality-ring-s3-v1':
+                    pathStyle = true; // fallthrough


I would recommend not relying on fallthrough capability of switch, instead using a map of properties per location name ({ 'location-scality-bleh': { pathStyle: true, ...} }) would be more readable and less error-prone.

alexanderchan-scality · 2018-04-13T23:15:44Z

@ironman-machine try
RUN_BACKBEAT_CRR_TESTS=true

ironman-machine · 2018-04-13T23:15:46Z

Hello @alexanderchan-scality

"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/21189' with the following env. args:

{
    "DEFAULT_BRANCH": "master",
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "REPO_NAME": "S3",
    "SCALITY_S3_BRANCH": "ft/more-s3-locations-support"
}

"RUN_BACKBEAT_CRR_TESTS=true": Success

alexanderchan-scality · 2018-04-13T23:18:18Z

@ironman-machine
RUN_BACKBEAT_CRR_TESTS=true
try

ironman-machine · 2018-04-13T23:18:20Z

Hello @alexanderchan-scality

"RUN_BACKBEAT_CRR_TESTS=true": Success
"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/21190' with the following env. args:

{
    "DEFAULT_BRANCH": "master",
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "REPO_NAME": "S3",
    "SCALITY_S3_BRANCH": "ft/more-s3-locations-support",
    "RUN_BACKBEAT_CRR_TESTS": "true"
}

ironman-machine · 2018-04-14T00:04:25Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-16T18:48:20Z

PR has been updated. Reviewers, please be cautious.

jonathan-gramain · 2018-04-16T19:01:37Z

lib/management/configuration.js

                    break;
-                case 'location-do-spaces-v1':
+                case 'location-scality-ring-s3-v1':
+                    pathStyle = true; // fallthrough


Reporting the style comment about using a mapping object rather than relying on fallthrough of a switch.

It's a good cleanup, we'll follow up on it

ironman-machine · 2018-04-17T00:22:52Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-17T00:22:53Z

CONFLICT (add/add): Merge conflict in lib/management/configuration.js
CONFLICT (add/add): Merge conflict in docker-entrypoint.sh

ironman-machine · 2018-04-17T17:07:08Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-17T18:27:49Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-17T23:46:38Z

PR has been updated. Reviewers, please be cautious.

rahulreddy force-pushed the master branch from 62729d2 to 0fd0493 Compare April 13, 2018 01:02

rahulreddy requested review from bennettbuchanan and philipyoo April 13, 2018 01:30

alexanderchan-scality force-pushed the ft/more-s3-locations-support branch from 0dd2a45 to fdabde3 Compare April 13, 2018 16:51

alexanderchan-scality force-pushed the ft/more-s3-locations-support branch from fdabde3 to 4011d20 Compare April 13, 2018 16:54

philipyoo reviewed Apr 13, 2018

View reviewed changes

philipyoo previously approved these changes Apr 13, 2018

View reviewed changes

bennettbuchanan previously approved these changes Apr 13, 2018

View reviewed changes

jonathan-gramain previously approved these changes Apr 13, 2018

View reviewed changes

rahulreddy dismissed stale reviews from jonathan-gramain, bennettbuchanan, and philipyoo via c30f901 April 14, 2018 00:04

rahulreddy force-pushed the ft/more-s3-locations-support branch from 4011d20 to c30f901 Compare April 14, 2018 00:04

rahulreddy previously approved these changes Apr 14, 2018

View reviewed changes

rachedbenmustapha dismissed rahulreddy’s stale review via fb2a823 April 16, 2018 18:48

rachedbenmustapha force-pushed the ft/more-s3-locations-support branch from c30f901 to fb2a823 Compare April 16, 2018 18:48

rahulreddy previously approved these changes Apr 16, 2018

View reviewed changes

philipyoo previously approved these changes Apr 16, 2018

View reviewed changes

jonathan-gramain reviewed Apr 16, 2018

View reviewed changes

jonathan-gramain previously approved these changes Apr 16, 2018

View reviewed changes

alexanderchan-scality dismissed stale reviews from jonathan-gramain and philipyoo via 7409532 April 17, 2018 00:22

alexanderchan-scality dismissed rahulreddy’s stale review via 7409532 April 17, 2018 00:22

alexanderchan-scality force-pushed the ft/more-s3-locations-support branch from 7409532 to 09a5f3d Compare April 17, 2018 17:07

alexanderchan-scality force-pushed the ft/more-s3-locations-support branch from 09a5f3d to bf3a60f Compare April 17, 2018 18:27

rachedbenmustapha previously approved these changes Apr 17, 2018

View reviewed changes

rachedbenmustapha and others added 6 commits April 17, 2018 16:46

Configure versioning and path style for s3 locations

6907df7

Make versioning optional for aws_s3 locations

fd8905a

Rely on location for PutBucketVersioning

822a2ce

Handle http s3 endpoints

aada239

healthcheck: versioning optional

dad7ee2

add supportsVersioning unit tests

460dd0d

alexanderchan-scality dismissed rachedbenmustapha’s stale review via 460dd0d April 17, 2018 23:46

alexanderchan-scality force-pushed the ft/more-s3-locations-support branch from bf3a60f to 460dd0d Compare April 17, 2018 23:46

philipyoo approved these changes Apr 18, 2018

View reviewed changes

bennettbuchanan approved these changes Apr 18, 2018

View reviewed changes

jonathan-gramain approved these changes Apr 18, 2018

View reviewed changes

rahulreddy merged commit eb97296 into master Apr 18, 2018

rahulreddy deleted the ft/more-s3-locations-support branch April 18, 2018 22:40

Ft/more s3 locations support #1207

Ft/more s3 locations support #1207

Uh oh!

Conversation

alexanderchan-scality commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ironman-machine commented Apr 13, 2018

Uh oh!

ironman-machine commented Apr 13, 2018

Uh oh!

philipyoo left a comment

Choose a reason for hiding this comment

Uh oh!

philipyoo Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

alexanderchan-scality Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

jonathan-gramain left a comment

Choose a reason for hiding this comment

Uh oh!

jonathan-gramain Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

alexanderchan-scality commented Apr 13, 2018

Uh oh!

ironman-machine commented Apr 13, 2018

Uh oh!

alexanderchan-scality commented Apr 13, 2018

Uh oh!

ironman-machine commented Apr 13, 2018

Uh oh!

ironman-machine commented Apr 14, 2018

Uh oh!

ironman-machine commented Apr 16, 2018

Uh oh!

jonathan-gramain Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

rachedbenmustapha Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

alexanderchan-scality commented Apr 13, 2018 •

edited

Loading