Default region should be configurable

[innovate-invent]

minio-js/src/main/minio.js

Line 561 in b859a03

if (!region) region = 'us-east-1'

I would prefer that the default region be configurable in the constructor.
The constructor already has a region parameter, should it default to that or should a defaultRegion parameter be added?

[harshavardhana]

just use the required region in the constructor

[innovate-invent]

just use the required region in the constructor

This function doesn't make use of that variable other than validation:

minio-js/src/main/minio.js

Lines 515 to 563 in b859a03

	makeBucket(bucketName, region, cb) {
	if (!isValidBucketName(bucketName)) {
	throw new errors.InvalidBucketNameError('Invalid bucket name: ' + bucketName)
	}
	if (isFunction(region)) {
	cb = region
	region = ''
	}
	if (!isString(region)) {
	throw new TypeError('region should be of type "string"')
	}
	if (!isFunction(cb)) {
	throw new TypeError('callback should be of type "function"')
	}
	var payload = ''
	// Region already set in constructor, validate if
	// caller requested bucket location is same.
	if (region && this.region) {
	if (region !== this.region) {
	throw new errors.InvalidArgumentError(`Configured region ${this.region}, requested ${region}`)
	}
	}
	// sending makeBucket request with XML containing 'us-east-1' fails. For
	// default region server expects the request without body
	if (region && region !== 'us-east-1') {
	var createBucketConfiguration = []
	createBucketConfiguration.push({
	_attr: {
	xmlns: 'http://s3.amazonaws.com/doc/2006-03-01/'
	}
	})
	createBucketConfiguration.push({
	LocationConstraint: region
	})
	var payloadObject = {
	CreateBucketConfiguration: createBucketConfiguration
	}
	payload = Xml(payloadObject)
	}
	var method = 'PUT'
	var headers = {}
	if (!region) region = 'us-east-1'
	this.makeRequest({method, bucketName, headers}, payload, 200, region, false, cb)
	}

[innovate-invent]

@harshavardhana please remove the 'working as intended' tag as the code does not reflect the intended functionality

[harshavardhana]

@innovate-invent it does, that's why its kept as is. makeBucket is meant to take custom region not default to the constructor. The constructor region is when makeBucket has been already done and you are using this to override region for your bucket lookup

[innovate-invent]

How do you make buckets in the constructor specified region without calling makeBucket?

[harshavardhana]

How do you make buckets in the constructor specified region without calling makeBucket?

Pass the same region in MakeBucket!

[innovate-invent]

haha, the point is to provide a default in the constructor so I don't have to pass it around in the code.

[harshavardhana]

haha, the point is to provide a default in the constructor so I don't have to pass it around in the code.

makeBucket is allowed to create buckets for different regions regardless of your default region in the constructor, so the code is nothing different. You can pass it just as you did for your constructor since you wish to keep this as default.

makeBucket is special so it is treated as a special operation.

[innovate-invent]

Yes, but that requires passing the region variable to every piece of code that calls makebucket.
Why not just provide a means of setting a default for each client instance?
This just adds pollution to every function signature that accepts a client object.

[harshavardhana]

I don't think that is idiomatic in nature because makeBucket() without region means its us-east-1 not your constructor default region. This is to behave as per S3 spec.

makebucket is a rare operation so if you are doing it frequently you have an architectural issue.

[innovate-invent]

It isn't that I am calling it frequently, it is that I am calling it several layers deep in the call stack compared to the instantiation of the client.
If it is simply a matter of strictly adhering to a specification, then I guess there is nothing to be done.