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

feature(query-params): Pass QueryParams through EdgeX to Device Services #1571

Merged

Conversation

rsdmike
Copy link
Member

@rsdmike rsdmike commented Jul 22, 2019

This pull request is an example/POC that @lenny-intel and I put together of how #1564 could be accomplished. This is only 1/2 of the equation as the device-sdk would need to be updated to handle the query parameters. The good thing with this is that it wouldn't be a breaking change as the device-sdk would simply ignore the query parameters that are sent on.

Signed-off-by: Mike Johanson michael.johanson@intel.com

@tsconn23 tsconn23 added the hold Intended for PRs we want to flag for ongoing review label Jul 22, 2019
@tsconn23
Copy link
Member

@ernestojeda I assume you're working on the ARM build?

13:31:26 standard_init_linux.go:190: exec user process caused "exec format error"
13:31:26 Makefile:171: recipe for target 'raml_verify' failed
13:31:26 make: *** [raml_verify] Error 1
13:31:26 Build step 'Execute shell' marked build as failure

@rsdmike rsdmike changed the title Pass QueryParams on through EdgeX to Device Services feature(query-params): Pass QueryParams through EdgeX to Device Services Jul 22, 2019
Copy link
Member

@tsconn23 tsconn23 left a comment

Choose a reason for hiding this comment

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

I've asked @rsdmike to extend the unit tests with some positive cases where the querystring isn't empty and also to verify we're correctly guarding against malformed querystrings.

Will review again when updated. Otherwise LGTM. Functional test was successful.

@tsconn23 tsconn23 removed the hold Intended for PRs we want to flag for ongoing review label Aug 5, 2019
@codecov-io
Copy link

codecov-io commented Aug 5, 2019

Codecov Report

Merging #1571 into master will decrease coverage by 0.91%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1571      +/-   ##
==========================================
- Coverage    29.4%   28.49%   -0.92%     
==========================================
  Files         125      124       -1     
  Lines       11000    10998       -2     
==========================================
- Hits         3235     3134     -101     
- Misses       7594     7697     +103     
+ Partials      171      167       -4
Impacted Files Coverage Δ
internal/core/command/device.go 0% <0%> (ø) ⬆️
internal/core/command/restDevice.go 0% <0%> (ø) ⬆️
internal/core/command/get.go 100% <100%> (+15.38%) ⬆️
internal/core/metadata/rest_addressable.go 47.94% <0%> (-45.46%) ⬇️
...rnal/core/metadata/operators/addressable/delete.go

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 1ae3ef6...b081903. Read the comment docs.

@rsdmike rsdmike force-pushed the supportCommandQueryParamsForGet branch from 836aa9d to 59f9d02 Compare August 5, 2019 20:31
Copy link
Member

@tsconn23 tsconn23 left a comment

Choose a reason for hiding this comment

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

LGTM.

Note, I think the Codecov.io report is in error. The negatively affected files which are attributable to the nearly %1 drop aren't even part of this PR. I think the commit history in the feature branch has thrown off the report. If that turns out to not be the case, will revisit with @rsdmike in the morning.

@tsconn23 tsconn23 merged commit 4d7ed08 into edgexfoundry:master Aug 6, 2019
rsdmike and others added 2 commits August 6, 2019 01:31
This pull request is an example/POC that @lenny-intel and I put together of how edgexfoundry#1564 could be accomplished. This is only 1/2 of the equation as the device-sdk would need to be updated to handle the query parameters. The good thing with this is that it wouldn't be a breaking change as the device-sdk would simply ignore the query parameters that are sent on.

Addresses edgexfoundry#1564

Signed-off-by: Mike Johanson <michael.johanson@intel.com>
jim-wang-intel pushed a commit to jim-wang-intel/edgex-go that referenced this pull request Aug 8, 2019
…ces (edgexfoundry#1571)

Fix edgexfoundry#1564

This pull request is an example/POC that @lenny-intel and I put together of how edgexfoundry#1564 could be accomplished. This is only 1/2 of the equation as the device-sdk would need to be updated to handle the query parameters. The good thing with this is that it wouldn't be a breaking change as the device-sdk would simply ignore the query parameters that are sent on.

Signed-off-by: Mike Johanson <michael.johanson@intel.com>
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.

3 participants