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 support for created and expires values #110

Merged

Conversation

blacktemplar
Copy link
Contributor

Resolves #109.

Adds support for created and expires values in parser and signer.

@zetxx
Copy link

zetxx commented Apr 8, 2020

+1 is anyone on this ?

@jasonbking
Copy link
Contributor

I'll try to get this reviewed sometime the next several days..

Copy link
Contributor

@jasonbking jasonbking left a comment

Choose a reason for hiding this comment

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

Aside from the one question, please run 'make check' -- there's a number of small lint + style errors it is flagging (that's the Jenkins failure). That will need to be clean before it can be merged.

Also, would you be willing to add a couple of tests since you're adding more logic to the parsing? I think a test that correctly parses a number parameter, as well as one that correctly catches an incorrectly formatted number (e.g. param=123foo456 or such) would be fine.

lib/signer.js Outdated Show resolved Hide resolved
@blacktemplar
Copy link
Contributor Author

I fixed all make check errors and added some new tests, testing the parsing behavior.

make test runs through except for one test (not ok 10 test/examples.test.js) which I think is unrelated to my pull request and also fails on my machine on the master branch.

But the CI still fails, so I am not sure whats the fault.

PS: would be good to document somewhere that one needs to install the npm package json globally to use make check, at least I didn't find it anywhere and needed some googling.

@jasonbking jasonbking merged commit 4fda5d0 into TritonDataCenter:master Apr 20, 2020
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.

Add support for created and expires values
3 participants