-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: new config format and parser #485
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
@KhudaDad414 I am trying to change the format for the config-file
now that the auth
parameters are being removed.
export default async function () { | ||
return { | ||
glee: {}, | ||
kafka: {}, | ||
websocket: {}, | ||
mqtt: {}, | ||
cluster: {}, | ||
http: {} | ||
docs: {}, | ||
server: {}, | ||
cluster: {} | ||
} | ||
} |
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.
This was the old format for glee-config
and it was done to configure WebSocket servers for socket-io or pass in a custom server object. So building on that I am trying to add a new object named server
where we would pass in the custom server config. The parser would take a server name from the spec and see if any config exists in the config file and then load that accordingly.
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.
It looks good, the only thing that worries me a little about this approach is writing an interface for it. I mean we want type suggestion/safety on the file right? with server name as a key I am not sure how we can write protocol specific interfaces. 🤔
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 we can still achieve that, I am not sure but I used notion API which had something like this kind of type
interface T {
server: {
[name:string]: {
httpAdapter: any,
adapter: string,
port: number
}
}
}
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.
@KhudaDad414 can I move ahead with 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.
yeah, If type can be checked then let's do this. 👍
This pull request has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation. There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
@Souvikns what are we going to do with this? 🤔 |
Since the config file has changed a lot, I am closing this and will open a new PR. |
Description
Since we are moving the client auth from the
config file
toauth
folder we need to update our config file format and update the parser as well to fetch these data differently.This PR is trying to achieve two things:
Related issue(s)
Fixes #392