This repository has been archived by the owner on Apr 11, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: initial structure+first approach #3
feat: initial structure+first approach #3
Changes from 3 commits
955ce81
eb43a6a
0eb2b6b
05f0988
d508b88
c5a3dbc
d126959
4d5a82b
92860f9
6268618
21adc59
951c291
e7dddea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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()
functionThere 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 addserverName
inconst 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 likeserver/{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.
Yeah! you're right. I just checked out the specs and saw that we could have it like this(
server/{parameter}
) too.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.
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 👍🏽