-
Notifications
You must be signed in to change notification settings - Fork 16
feat: initial structure+first approach #3
feat: initial structure+first approach #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
Hey @arjungarg07 if you want the PR's reviewed this week feel free to request a review by me or any of the other maintainers as Lukasz is on a small holiday, as I assume you know 🙂 |
Hey @jonaslagoni Yeah, would love it to be reviewed so that it can get merged and I can move forward. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, only some small suggestion, take them with a gram of salt. If you have agreed something different with the GSOC mentor (what to focus on etc) feel free to disregard the comments 🙂
I would have loved to see some tests alongside the initial structure, have you discussed with @derberg what the focus is on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! I need some changes 😅 Please refer to them :) If you have any question, write them! :)
lib/appRelationsDiscovery.js
Outdated
if (doc.hasServers()) { | ||
const servers = doc.servers(); | ||
|
||
for (const [, credentials] of Object.entries(servers)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need only values you can use .values()
function
for (const [, credentials] of Object.entries(servers)) { | |
for (const credentials of Object.values(servers)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had it like for (const [serverName, credentials] of Object.entries(servers))
and was thinking to add serverName
in const slug = `${credentials.url()},${credentials.protocol()}
so as to make it more unique. Like what if two servers named 'production' and 'development' have the same url and protocol, in that case, slug would remain same and services would double up in the sub, pub Array.
But due to lint issue, removed that for now :)
So do we have to make slug more unique having the names of servers concatenated too like:
slug = serverName,${credentials.url()},${credentials.protocol()}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But serverName
is only a name for server, it's something like metadata for server, it can be named in different way in other spec, so as you wrote you can end with double items in map for this same server. I think that we should operate only on url and protocol. From your comment I also find some problems, because someone can describe server's url with parameters like server/{parameter}
and define for this parameter some enum values. Not now, but in the next weeks (maybe at the end of gsoc) we should check if this server is exactly this same as in another spec with given possible enum. I hope that you understand :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should operate only on url and protocol
someone can describe server's url with parameters likeserver/{parameter}
and define for this parameter some enum values.
Yeah! you're right. I just checked out the specs and saw that we could have it like this(server/{parameter}
) too.
servers:
- url: '{username}.gigantic-server.com:{port}/{basePath}'
description: The production API server
protocol: secure-mqtt
variables:
username:
# note! no enum here means it is an open value
default: demo
description: This value is assigned by the service provider, in this example `gigantic-server.com`
port:
enum:
- '8883'
- '8884'
default: '8883'
basePath:
# open meaning there is the opportunity to use special base paths as assigned by the provider, default is `v2`
default: v2
This might be a big concern coz like here, we have to configure the flow to support this case too, also need to iterate with multiple ports: 8883, 8884
. I will keep this in mind, and would definitely fix it in the next weeks. For now, I think we can move forward. What do you suggest? 🤔
Shall I create an issue now only regarding this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you should create issue for that to not forget about it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create issue for that and we can discuss about it in the new issue :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you misunderstood me here. What you described is totally clear to me.
But, I was talking about a case like this one.
#one spec
servers:
production:
url: mqtt://localhost:1883
protocol: mqtt
development:
url: mqtt://localhost:1883
protocol: mqtt
channels:
someAnotherChannel: ...
If this is the case then we would iterate 2 times(for production and development) having the same slug and then double up the applications described for this AsyncAPI document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaa ok, sorry :) But there you have also this same problem, because you will have doubled servers and you won't be able to connect in easy way this app with another app in another spec. I think that we should go first with only url and protocol and then think about serverName. You can create issue for that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh sorry... I think about serverName
again and again... Of course, you're right, in current logic you will have doubled pub/sub in your app, but I don't think so that it should be hard to fix, but not now :) You can create issue for that and handle it in the next PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it could be a potential bug coz user should never have define the spec having two servers with same url and protocol in the first place. Would open an issue for the same 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me almost is good, please only write in one place try ... catch
and also add (as Jonas wants) JSDoc for getRelations
and validate
functions :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only two adds suggestion to JSDoc comments, except that everything for me is good. Awesome work! :)
Co-authored-by: Maciej Urbańczyk <[email protected]>
Co-authored-by: Maciej Urbańczyk <[email protected]>
Thanks for reviewing the PR, really 🙏🏽 |
@magicmatatjahu Can we merge this one? Also, can I get access to this repo, I would like to set up the project board :) |
@arjungarg07 Sure, no problem. I will accept PR and ping Jonas :)
I don't have permission to add permissions to another person, but I created board for you https://github.com/asyncapi/app-relations-discovery/projects/1 For permissions, please wait for Łukasz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Awesome work! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add some release-specific configuration to the package.json file 🙂
Co-authored-by: Jonas Lagoni <[email protected]>
Co-authored-by: Jonas Lagoni <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🎉
🎉 This PR is included in version 0.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR will:
getRelations
API for default syntaxdefault Syntax:
Output Example: