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 Code Query Parameter #99

Merged
merged 2 commits into from
Mar 14, 2018
Merged

Add Code Query Parameter #99

merged 2 commits into from
Mar 14, 2018

Conversation

dubee
Copy link
Member

@dubee dubee commented Mar 4, 2018

Adds an optional query parameter for action fetches that determines if action code will be retrieved or not.

@dubee dubee added the wip label Mar 4, 2018
@dubee dubee force-pushed the code-query branch 6 times, most recently from 68695b9 to 81d14bc Compare March 4, 2018 07:39
@dubee dubee requested a review from jthomas March 4, 2018 07:40
@dubee dubee added review and removed wip labels Mar 4, 2018
@dubee
Copy link
Member Author

dubee commented Mar 4, 2018

@jthomas, I am seeing a trigger failure in Travis. Any idea why that test is failing?

  ✖ triggers › fire a trigger   
  t.true(update_result.hasOwnProperty('activationId'))
                       |                              
                       false    

Copy link
Member

@jthomas jthomas left a comment

Choose a reason for hiding this comment

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

Code looks perfect. README needs updating with the new parameter details. Add a sentence or two @ https://github.com/apache/incubator-openwhisk-client-js#retrieve-resource

@@ -60,7 +79,7 @@ test('should retrieve action from string identifier', t => {
t.is(path, `namespaces/${ns}/actions/12345`)
}

return actions.get('12345')
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to make this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jthomas, yes this change was needed to prevent an error now that action.get doesn't use the default get method from resources.js.

Copy link
Member

Choose a reason for hiding this comment

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

@dubeejw The test case was throwing an error because this change broke that API. Can you submit a PR to revert this test case change and modify the get method to handle the string identifier. There's a parse_options method that you can use to handle the string case correctly. https://github.com/apache/incubator-openwhisk-client-js/blob/master/lib/resources.js#L64-L70

We'll push this out as a patch release.

@jthomas
Copy link
Member

jthomas commented Mar 13, 2018

Can you re-base this PR once #102 is merged to resolve the build issue?

@codecov-io
Copy link

codecov-io commented Mar 13, 2018

Codecov Report

Merging #99 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage   99.16%   99.17%   +<.01%     
==========================================
  Files          14       14              
  Lines         361      364       +3     
==========================================
+ Hits          358      361       +3     
  Misses          3        3
Impacted Files Coverage Δ
lib/actions.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80075e3...9b17e3c. Read the comment docs.

@dubee
Copy link
Member Author

dubee commented Mar 13, 2018

@jthomas, Travis is passing for this one now.

@csantanapr csantanapr dismissed jthomas’s stale review March 14, 2018 02:52

readme updated now

Copy link
Member

@csantanapr csantanapr left a comment

Choose a reason for hiding this comment

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

LGTM

@csantanapr csantanapr merged commit 9dfcd1a into apache:master Mar 14, 2018
@csantanapr csantanapr mentioned this pull request Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants