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

MF-785 - Change CanAccess to CanAccessByKey #894

Merged
merged 5 commits into from
Oct 21, 2019
Merged

MF-785 - Change CanAccess to CanAccessByKey #894

merged 5 commits into from
Oct 21, 2019

Conversation

nwneisen
Copy link
Contributor

What does this do?

Change CanAccess code to use CanAccessByKey

Which issue(s) does this PR fix/relate to?

Resolves #785

List any changes that modify/break current functionality

Endpoint /channels/<channel id>/access was changed to /channels/<channel id>/access-by-key. This could cause issues if it was being access outside of the SDK. It didn't seem to be exposed.

Have you included tests for your changes?

No new tests were created. Current tests were verified to still be passing.

Did you document any new/modified functionality?

No, new functionality was not added and this endpoint is not documented.

Notes

Signed-off-by: Nick Neisen <nwneisen@gmail.com>
Signed-off-by: Nick Neisen <nwneisen@gmail.com>
Signed-off-by: Nick Neisen <nwneisen@gmail.com>
@drasko
Copy link
Contributor

drasko commented Oct 21, 2019

@nwneisen great initiative.

However this change will probably impact Aedes and VerneMQ MQTT adapters, as they depend on protobuf (which was changed).

Can you please verify if this is the case, and do the necessary changes over there as well (at least for Aedes, and I can do Erlang stuff for VerneMQ later).

@dborovcanin
Copy link
Collaborator

@nwneisen great initiative.

However this change will probably impact Aedes and VerneMQ MQTT adapters, as they depend on protobuf (which was changed).

Can you please verify if this is the case, and do the necessary changes over there as well (at least for Aedes, and I can do Erlang stuff for VerneMQ later).

@nwneisen Also, please generate .pb.go files. We commit these files to simplify CI and to track proto files easily. That's also the reason CI is failing.

@drasko
Copy link
Contributor

drasko commented Oct 21, 2019

@dusanb94 these seem to be commited.

Also I double-checked that Verne does not use CanAccessByKey() (it uses only CanAccessByID(), which was the reason for introducing it and making these distinctions).

However, I think that Aedes calls need to be adjusted.

@drasko
Copy link
Contributor

drasko commented Oct 21, 2019

On double-checking PR Aedes code seems to be handled as well.

I have good impression about this PR ;).

nmarcetic
nmarcetic previously approved these changes Oct 21, 2019
drasko
drasko previously approved these changes Oct 21, 2019
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko
Copy link
Contributor

drasko commented Oct 21, 2019

@nwneisen I tried updating your branch via GitHUb UI button (as you have maintainer approvals, and we would like to merge this PR). However, CI is falling - probably because wrong protoc version.

Can you try updating the branch manually to see if we can fix this issue?

Our CI script seems to use this version of protoc. We should either bump it, remove this check, or you must for now align with what CI does.

@dusanb94 can we take a look at bumping this protoc version, looks like 3.10 is actual one.

@nwneisen nwneisen dismissed stale reviews from drasko and nmarcetic via f85143e October 21, 2019 19:15
@codecov-io
Copy link

codecov-io commented Oct 21, 2019

Codecov Report

Merging #894 into master will decrease coverage by 0.04%.
The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #894      +/-   ##
==========================================
- Coverage   84.58%   84.54%   -0.05%     
==========================================
  Files          74       74              
  Lines        4879     4879              
==========================================
- Hits         4127     4125       -2     
- Misses        511      513       +2     
  Partials      241      241
Impacted Files Coverage Δ
things/redis/streams.go 90.47% <0%> (ø) ⬆️
things/api/auth/grpc/client.go 100% <100%> (ø) ⬆️
things/service.go 86.61% <100%> (-1.41%) ⬇️
things/api/auth/grpc/endpoint.go 100% <100%> (ø) ⬆️
ws/api/transport.go 87.5% <100%> (ø) ⬆️
things/api/auth/http/endpoint.go 100% <100%> (ø) ⬆️
things/api/auth/http/requests.go 100% <100%> (ø) ⬆️
readers/api/transport.go 87.01% <100%> (ø) ⬆️
things/api/auth/grpc/requests.go 66.66% <100%> (ø) ⬆️
things/api/auth/grpc/server.go 96.49% <100%> (ø) ⬆️
... and 2 more

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 1da48af...6fe26e6. Read the comment docs.

Signed-off-by: Nick Neisen <nwneisen@gmail.com>
@nwneisen
Copy link
Contributor Author

@drasko I checked my protoc version and I was indeed on 3.9.1. It looks like the build is now successful and I just needed to push with a signed commit.

Copy link
Contributor

@drasko drasko 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

@manuio manuio left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit 6687a73 into absmach:master Oct 21, 2019
@drasko
Copy link
Contributor

drasko commented Oct 21, 2019

Merged! Thanks @nwneisen!

@nwneisen nwneisen deleted the MF-785 branch October 29, 2019 18:19
manuio pushed a commit that referenced this pull request Oct 12, 2020
* Change CanAccess to CanAccessByKey for things

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

* Change CanAccess in remaining occurances

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

* Regenerate generated files

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

* Generate pb.go files with protoc 3.6.1

Signed-off-by: Nick Neisen <nwneisen@gmail.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.

Rename CanAccess endpoint to CanAccessByKey
6 participants