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: re add protobuff parser #758

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Conversation

GreenRover
Copy link
Contributor

re enable protobuff parser lib

@GreenRover GreenRover changed the title Feat: re add protobuff parser feat: re add protobuff parser Sep 6, 2023
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@GreenRover
Copy link
Contributor Author

/rtm

@smoya
Copy link
Member

smoya commented Sep 7, 2023

/rtm

@smoya
Copy link
Member

smoya commented Sep 7, 2023

Would it make sense to merge this into master directly? Supporting protobuf here doesn't need to wait anything else isn't?

@GreenRover
Copy link
Contributor Author

It can go in the master as well. But this pr is not compatible with master.
Is there a release timeline for next major?

@smoya
Copy link
Member

smoya commented Sep 7, 2023

I see the next branch is using the ^3.0.0-next-major-spec.1" version of the parser. If instead we use the latest release (not prerelease) then would be able to merge to master I guess. Is there any strong reason this to be blocked by the release of v3 of the spec? Pinging @jonaslagoni who worked on the new parser adoption in this repo.

@jonaslagoni
Copy link
Member

Lets just do the change to next, its the only place where it makes sense.

And it already is there on master, so no need to change anything there. Its only relevant on next because I had to disable it after the rewrite to parser v3.

@GreenRover GreenRover mentioned this pull request Sep 7, 2023
@GreenRover
Copy link
Contributor Author

@jonaslagoni protobuf-parser was not jet part in master. I only add it for next branch so far. Here the pr for master #759

@jonaslagoni
Copy link
Member

Ahh, right @GreenRover 👍

@GreenRover
Copy link
Contributor Author

Back to this issue. What needs to be done to get it merged?

@jonaslagoni
Copy link
Member

I really dont see it as necessary to add it to master as well, but of course if you use v0 somewhere it would be nice to have 😄

@GreenRover
Copy link
Contributor Author

Me neither, i would prefer to get next released because it fixes many of my open issue with current release.

@sonarcloud
Copy link

sonarcloud bot commented Sep 7, 2023

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

@fmvilas fmvilas merged commit 15de3c0 into asyncapi:next Sep 7, 2023
9 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0-next.53 🎉

The release is available on:

Your semantic-release bot 📦🚀

@GreenRover
Copy link
Contributor Author

@derberg
Copy link
Member

derberg commented Sep 12, 2023

@GreenRover I think it is cause all by outdated pipelines -> https://github.com/asyncapi/asyncapi-react/actions/runs/6111938173/job/16588288332

let's just release -> #761

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.

6 participants