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

Add requestDate parameter to presignedUrl() and presignedGetObject() #728

Merged
merged 8 commits into from
Dec 4, 2018

Conversation

dustin-H
Copy link
Contributor

Hi there,

I just added a parameter requestDate to the methods presignedUrl() and presignedGetObject().

Please have a look at #724 for more details.

My todo checklist:

  • Adjust the code
  • Add js-doc parameter description
  • Create new tests for the functionality
  • All (old and new) tests should pass
  • Update docs
  • Create example

docs/API.md Outdated Show resolved Hide resolved
docs/API.md Outdated
@@ -761,6 +762,8 @@ __Parameters__
|`bucketName` | _string_ | Name of the bucket. |
|`objectName` | _string_ | Name of the object. |
|`expiry` | _number_ | Expiry time in seconds. Default value is 7 days. |
|`respHeaders` | _object_ | response headers to override (optional) |
|`requestDate` | _Date_ | A date object, the url will be issued at. Default value is now. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Title above also needs to be modified accordingly, line# 751:

### presignedGetObject(bucketName, objectName, expiry[, cb])
=>
### presignedGetObject(bucketName, objectName[, expiry, respHeaders, requestDate, cb])

(optinal) information should be added to the above expiry and requestDate descriptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 29b457a

docs/API.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

@dustin-H,
There is also a travis failure you need to address:

  let code = codeRegex();
  ^^^
SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode
    at NativeCompileCache._moduleCompile (/home/travis/.nvm/versions/node/v5.12.0/lib/node_modules/yarn/lib/v8-compile-cache.js:226:18)
    at Module._compile (/home/travis/.nvm/versions/node/v5.12.0/lib/node_modules/yarn/lib/v8-compile-cache.js:172:36)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Module.require (module.js:367:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/home/travis/.nvm/versions/node/v5.12.0/lib/node_modules/yarn/bin/yarn.js:24:13)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)

@dustin-H
Copy link
Contributor Author

dustin-H commented Nov 1, 2018

@ebozduman
Yes, I've seen this.

However, I think this is a yarn issue yarnpkg/yarn#6619.

I haven't changed anything regarding this.

@dustin-H
Copy link
Contributor Author

dustin-H commented Nov 1, 2018

@ebozduman
I fixed it by setting the yarn version in .travis.yml to [email protected].

This is the latest version, which has successfully passed the last travis build.

@ebozduman
Copy link
Contributor

@dustin-H,
Perfect. Great job!

Copy link
Contributor

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM

src/main/minio.js Show resolved Hide resolved
@dustin-H
Copy link
Contributor Author

dustin-H commented Nov 3, 2018

The build is currently failing, because a package down the dependency tree has published a major release (node-version changed) as patch.

I'm talking about graceful-fs

They changed from 0.4.11 to 0.4.12 the node min version: (Now the latest is 0.4.13)

https://cdn.jsdelivr.net/npm/[email protected]/package.json
https://cdn.jsdelivr.net/npm/[email protected]/package.json

I already created an issue: isaacs/node-graceful-fs#146

However, we may could prevent errors like this by creating a lockfile.

@dustin-H
Copy link
Contributor Author

dustin-H commented Nov 4, 2018

@ebozduman Now, everything is ready thanks to isaacs/node-graceful-fs#141 (comment).

Copy link
Contributor

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM

@dustin-H
Copy link
Contributor Author

dustin-H commented Dec 4, 2018

@ebozduman @krishnasrinivas

Do you know, when this will be merged. I really need this.

@kannappanr kannappanr merged commit a180e87 into minio:master Dec 4, 2018
@kannappanr
Copy link
Contributor

@dustin-H Thanks for the PR. This has been merged. We will make a release in the next few days.

@dustin-H
Copy link
Contributor Author

dustin-H commented Dec 4, 2018

Thanks you!

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.

4 participants