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.

Sign up for GitHub

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

Jump to bottom

MF-1091 - Use channels. as broker prefix #1098

Merged
merged 2 commits into from
Apr 6, 2020
Merged

MF-1091 - Use channels. as broker prefix #1098

merged 2 commits into from
Apr 6, 2020

Conversation

manuio
Copy link
Contributor

@manuio manuio commented Apr 4, 2020 •
edited
Loading

Signed-off-by: Manuel Imperiale manuel.imperiale@gmail.com

  • Use "channels" instead of "channel" as subject prefix of the NATS message broker.

@manuio manuio requested a review from a team as a code owner April 4, 2020 17:05
drasko
drasko requested changes Apr 4, 2020
View reviewed changes
broker/nats.go Outdated Show resolved Hide resolved
@drasko
Copy link
Contributor

drasko commented Apr 4, 2020

This is an API breaking change for all applications that are using data directly from the NATS, and this should be documented for the new release.

@manuio
MF-1091 - Use channels. as broker prefix
afb188d 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio
Copy link
Contributor Author

manuio commented Apr 5, 2020

@drasko docs updated: absmach/magistrala-docs#22

drasko
drasko approved these changes Apr 5, 2020
View reviewed changes
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 drasko mentioned this pull request Apr 5, 2020
Merged
nmarcetic
nmarcetic requested changes Apr 5, 2020
View reviewed changes
broker/nats.go
// SubjectAllChannels define the subject to subscribe to all channels messages
SubjectAllChannels = "channel.>"
SubjectAllChannels = "channels.>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please describe this PR, the purpose and reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nmarcetic this is more or less evident. channels.<chan_id> is to make it consistent with the public API (which is channels/<chan_id>, and not channel/<chan_id>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case I updated the description of the PR

@manuio
Fix conflicts
03099b4 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #1098 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1098   +/-   ##
=======================================
  Coverage   77.10%   77.10%           
=======================================
  Files          95       95           
  Lines        6760     6760           
=======================================
  Hits         5212     5212           
  Misses       1211     1211           
  Partials      337      337

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 1d78233...03099b4. Read the comment docs.

@drasko drasko merged commit 8325c1c into absmach:master Apr 6, 2020
@manuio manuio deleted the subject branch April 6, 2020 11:08
@manuio manuio mentioned this pull request Apr 6, 2020
Closed
@jonathandreyer jonathandreyer mentioned this pull request Apr 6, 2020
Closed
manuio added a commit that referenced this pull request Oct 12, 2020
@manuio
MF-1091 - Use channels. as broker prefix (#1098)
d854eee 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drasko drasko drasko approved these changes

@nmarcetic nmarcetic nmarcetic approved these changes

@dborovcanin dborovcanin dborovcanin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@manuio @drasko @codecov-io @nmarcetic @dborovcanin