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

feat: add /bundle endpoint #70

Merged
merged 13 commits into from
Mar 29, 2022
Merged

Conversation

Samridhi-98
Copy link
Contributor

Description

  • Updated openapi.yaml
  • Modified controler and package.json

Related issue(s)

Fixes #55

Copy link

@github-actions github-actions bot left a 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 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.

@magicmatatjahu
Copy link
Member

@Samridhi-98 Hi! Do you need some help? I know that PR is on draft stage but I see several problems, so I don't know if you wanna my review now or later :)

@Samridhi-98
Copy link
Contributor Author

@Samridhi-98 Hi! Do you need some help? I know that PR is on draft stage but I see several problems, so I don't know if you wanna my review now or later :)

Yeah, I need suggestions actually I made a few changes and haven't pushed yet that's why didn't ask.
Will you please address the problem🙂

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

@Samridhi-98 Awesome, but we need improve some things :) Please refer to my comments. When you propagate my suggestions, please ping me and I will tell you what you should do in next step to validate files/asyncapis field in proper way. Please don't do that in the main controller, please wait with it :)

openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
src/controllers/bundler.controller.ts Outdated Show resolved Hide resolved
src/controllers/bundler.controller.ts Outdated Show resolved Hide resolved
src/controllers/bundler.controller.ts Outdated Show resolved Hide resolved
src/controllers/bundler.controller.ts Outdated Show resolved Hide resolved
@Samridhi-98
Copy link
Contributor Author

@magicmatatjahu I have made the requested changes as per my understanding kindly let me know what should be the next step🙂

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Good, but we still need to improve a few things :)

openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated
Comment on lines 185 to 288
type: object
required:
- asyncapi
properties:
asyncapi:
$ref: "#/components/schemas/AsyncAPIDocument"
Copy link
Member

Choose a reason for hiding this comment

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

You can check this issue asyncapi/bundler#26 :) We should only pass files (asyncapis or single one file by asyncapi) and base object as optional field. asyncapis and asyncapi should be required (as union, only one should be sent).

openapi.yaml Outdated Show resolved Hide resolved
src/controllers/bundler.controller.ts Outdated Show resolved Hide resolved
src/controllers/bundler.controller.ts Outdated Show resolved Hide resolved
src/controllers/bundler.controller.ts Outdated Show resolved Hide resolved
src/controllers/bundler.controller.ts Outdated Show resolved Hide resolved
@magicmatatjahu magicmatatjahu changed the title feat: added /bundle feat: add /bundle endpoint Mar 10, 2022
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

We have still problems that we need resolve, but it's getting better. :)

src/controllers/bundler.controller.ts Outdated Show resolved Hide resolved
src/controllers/bundler.controller.ts Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
src/controllers/tests/bundler.controller.test.ts Outdated Show resolved Hide resolved
@Samridhi-98
Copy link
Contributor Author

@magicmatatjahu I am not able to figure out how to send data in a base field inside test☹

channels: {},
}
})
.expect(200);
Copy link
Member

Choose a reason for hiding this comment

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

@Samridhi-98 You properly tested it 👏🏼 Please also check response data and everything will be ok.

@Samridhi-98
Copy link
Contributor Author

image

@magicmatatjahu I am getting this error while checking for the response ☹

@Samridhi-98 Samridhi-98 marked this pull request as ready for review March 23, 2022 08:58
@magicmatatjahu
Copy link
Member

@Samridhi-98 Sorry for such a delay. We have have there a big problem with circular references in the openapi.yaml and one global middleware couldn't handle, so I had to make my own changes because they were too difficult for a person not familiar with the whole architecture. Take a look at the changes and see how the implementation should look. Let me know if everything ok and we'll merge it :)

@Samridhi-98
Copy link
Contributor Author

@magicmatatjahu Thank you so much for the help, it's working now 🙂

@magicmatatjahu
Copy link
Member

@Samridhi-98 Everything is ok, but I have to push one important commit on master first to make it possible to merge this PR, so please wait.

@Samridhi-98
Copy link
Contributor Author

@Samridhi-98 Everything is ok, but I have to push one important commit on master first to make it possible to merge this PR, so please wait.

No problem 😊

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@magicmatatjahu
Copy link
Member

@Samridhi-98 Thanks for contribution and patience! I hope you learned something new :)

@magicmatatjahu
Copy link
Member

/rtm

@asyncapi-bot
Copy link
Contributor

Hello, @magicmatatjahu! 👋🏼
This PR is not up to date with the base branch and can't be merged.
You can add comment to this PR with text: /autoupdate or /au. This way you ask our bot to perform regular updates for you. The only requirement for this to work is to enable Allow edits from maintainers option in your PR. Otherwise the only option that you have is to manually update your branch with latest version of the base branch.
Thanks 😄

@asyncapi-bot asyncapi-bot merged commit 81a2dee into asyncapi:master Mar 29, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Samridhi-98
Copy link
Contributor Author

@Samridhi-98 Thanks for contribution and patience! I hope you learned something new :)

Thank you so much for your help and guidance, Indeed great learning 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add implementation for /bundle path
3 participants