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

Move Spectral ruleset to this repo #698

Closed
magicmatatjahu opened this issue Jan 26, 2023 · 14 comments · Fixed by #790
Closed

Move Spectral ruleset to this repo #698

magicmatatjahu opened this issue Jan 26, 2023 · 14 comments · Fixed by #790
Labels
enhancement New feature or request

Comments

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jan 26, 2023

As you know (or not) in the ParserJS 2.X.X we switched to the Spectral project to validate AsyncAPI spec. For that we are using the official AsyncAPI ruleset (which we also maintain it). However, we have a problems with release process.

First of all, Spectral has its own release process and if we wanna release new minor version of spec and extend ruleset (to e.g. support new version or add new rules to validate new syntax in spec) we need to create PR in Spectral project, wait for merge and then bump Spectral in parser-js.

Another problem is related to the previous one, but it has more impact on the DX and UX. If we have split process of releasing parser-js and releasing Spectral, we cannot test new things (new syntax) in spec, for example in the CLI or Studio using the:

asyncapi: 2.6.0-rc-1
...

spec - especially for 3.0.0 release, and later for each new major versions. I mean, we can, but we have to wait for it.

I think that we should start writing all rules in this repository and adding new rules when we need them (or changing existing one).

Of course the AsyncAPI ruleset in Spectral will be outdated in some way, but we can always contribute new rules to the Spectral repo, however:

  • we need to remember that if we will remove given rule(s) or change the diagnostic type of it/them, Spectral will have breaking change on its side
  • if Spectral changes something inside code (make breaking change), we will have to also release new major version

Of course, we should inform the Spectral people about our idea, but I don't think they will have a problem with it. We should discuss that first in this issue, then inform them.

WDYT?

@jonaslagoni @smoya @fmvilas @derberg

@magicmatatjahu magicmatatjahu added the enhancement New feature or request label Jan 26, 2023
@jonaslagoni
Copy link
Member

I dont see any issue here besides you wanting to synchronize rules between spectral and our parser. IMO that should never be the case.

They should follow two completely separate release flows, spectral rules start in the parser, and can be proposed to the spectral library, but that does not necessarily mean they have to be accepted, cause it might be just a very specific use-case for us and the parser.

We should have no constraint to spectral other than that we use the library and would like to contribute back to it, with good rules that would make sense for others to use as well.

@derberg
Copy link
Member

derberg commented Jan 30, 2023

@jonaslagoni

cause it might be just a very specific use-case for us and the parser.

do you have an example? ideally this should never happen, parser should follow spec, and there should be no specific rules, or?

@jonaslagoni
Copy link
Member

jonaslagoni commented Jan 30, 2023

do you have an example? ideally this should never happen, parser should follow spec, and there should be no specific rules, or?

@derberg One that comes to mind is supported schema formats, where we might explicitly warn or error when unknown schema formats are used according to what the parser and schema parsers exist.

That rule is more or less 100% specific for this library.

Copy link
Member

derberg commented Jan 31, 2023

ok, but schema formats are still spec-related, so ideally we should not have a very specific use-case for us and the parser right? parser is for spec validation and parsing, nothing more.

@magicmatatjahu
Copy link
Member Author

ok, but schema formats are still spec-related, so ideally we should not have a very specific use-case for us and the parser right? parser is for spec validation and parsing, nothing more.

How do you want to incorporate the custom schema formats in Spectral, if we have such an information (in parser) after registration of custom parsers? We can say that given avro format/type is valid, but we should also check that given avro value is valid and it won't be good to make such a things in Spectral.

@jonaslagoni
Copy link
Member

jonaslagoni commented Jan 31, 2023

ok, but schema formats are still spec-related, so ideally we should not have a very specific use-case for us and the parser right? parser is for spec validation and parsing, nothing more.

It is spec-related yes, but it's implementation specific as well as tooling-specific. End-users that use spectral to ensure their AsyncAPI document complies with the design rules would never use such a rule, but instead, force a specific schema format. So yea as @magicmatatjahu says, its only related to our parser 🙂

I am by no means saying that such a rule should NOT be located in spectral at some point, but I suggest that it starts here, and then gets proposed as a rule in the Spectral library afterward.

@magicmatatjahu as I understand your entire suggestion is because you want the spectral rulesets to be more maintainable, and the best way to do this is to place them here in the parser. Am I right in that understanding?

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Feb 2, 2023

@jonaslagoni

as I understand your entire suggestion is because you want the spectral rulesets to be more maintainable, and the best way to do this is to place them here in the parser. Am I right in that understanding?

Yes :) So what? Should we merge #699? We have blocker in the stoplightio/spectral#2391 and we cannot support AsyncAPI 2.6.0 in cli and studio.

@derberg
Copy link
Member

derberg commented Feb 2, 2023

I totally support moving rules here.
I never liked the idea that we depend on spectral and that they cannot separate rules to separate package that could be hosted under asyncapi

and it won't be good to make such a things in Spectral.

but we anyway should not have any asyncapi rules related to json schema or avro or anything else. And even if some time we decide to enable them (which would be strange, we do not own these specs, there should be rules from given spec maintainers) they should be considered not asyncapi rules, but avro or json schema that we just use.

but I suggest that it starts here, and then gets proposed as a rule in the Spectral library afterward

yes, I agree with that


you know thought that once we take over ruleset and manage it here, pushing too upstream will not be something with high priority right? So I think that once we merge PR to move rules here, we should start discussion with Spectral and push to get spectral asyncapi rules to separate package under asyncapi org

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Feb 2, 2023

@derberg

you know thought that once we take over ruleset and manage it here, pushing too upstream will not be something with high priority right? So I think that once we merge PR to move rules here, we should start discussion with Spectral and push to get spectral asyncapi rules to separate package under asyncapi org

Yeah. This is not stealing intellectual value from their repo, I think 😅. Fact, some rules were previously written by other people, then we started the contribution, but ideally we should release new rules when we release a new spec (or some rc) and current setup is problematic. A separate package like @asyncapi/spectral with only rules should be the way out, but now we can have it located in parser source code.

@jonaslagoni
Copy link
Member

Yeah. This is not stealing intellectual value from their repo, I think 😅

It's apache-2 so it's not even a question 🙂

So I think that once we merge PR to move rules here, we should start discussion with Spectral and push to get spectral asyncapi rules to separate package under asyncapi org

Agree.

@derberg
Copy link
Member

derberg commented Feb 3, 2023

I see it this way:

  1. We introduce rules in parser and should transparently inform spectral folks about it
  2. We then move them to @asyncapi/spectral
  3. We continue the discussion as long as needed to make sure spectral also uses @asyncapi/spectral.

@jonaslagoni
Copy link
Member

That would be ideal, and if they don't want to use the package, then everyone else is still free to use it 🙂

@github-actions
Copy link

github-actions bot commented Jun 4, 2023

This issue 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 issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue 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 issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@smoya
Copy link
Member

smoya commented Jun 20, 2023

still valid as docs PR is not merged yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants