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

fix: add v3 checks for all commands #697

Merged
merged 8 commits into from
Jul 27, 2023

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Jul 24, 2023

Description
This PR adds v3 checks for most commands, and give an error if the command does not support v3:

  • bundle
  • diff, when one of the files are v3
  • generate fromTemplate, when the template is a specific one:
    • @asyncapi/html-template
    • @asyncapi/dotnet-nats-template
    • @asyncapi/ts-nats-template
    • @asyncapi/python-paho-template
    • @asyncapi/nodejs-ws-template
    • @asyncapi/java-spring-cloud-stream-template
    • @asyncapi/go-watermill-template
    • @asyncapi/java-spring-template
    • @asyncapi/markdown-template
  • generate models
  • optimize
  • validate

I left out the following commands, because I don't think they are relevant to shelter:

  • convert (already supports v3)
  • glee (has it's own runtime)
  • studio (has it's own runtime)

It does so by peaking into the spec document and checking the asyncapi property.

These checks are only temporary and another approach in the future is probably needed as discussed in #629, but this change is to ensure CLI is ready whenever v3 is released.

Related issue(s)
Related to #629

@jonaslagoni jonaslagoni marked this pull request as ready for review July 24, 2023 18:19
Comment on lines +135 to +138
if (v3IssueLink !== undefined) {
this.error(`${template} template does not support AsyncAPI v3 documents, please checkout ${v3IssueLink}`);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

what happens when I pass in a custom template that does not support v3?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing, we cant expect the unexpected 🙂

Custom templates can easily be added to the list of templates that don't support v3, so all about contributing 😄

Copy link
Member

Choose a reason for hiding this comment

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

Nothing, we cant expect the unexpected 🙂

Yeah, that make sense.

I was thinking that since templates have a config in the package.json they might have a parameter that could help us determine which spec versions this template supports, but now that I have checked I don't think there is anything like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the future for sure, as you folks discussed in #629, each tools should have that configured, but it would be a rush to implement it now right before v3, so this is just a temporary solution until something better comes along 🙂

Souvikns
Souvikns previously approved these changes Jul 25, 2023
Copy link
Member

@Souvikns Souvikns left a comment

Choose a reason for hiding this comment

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

LGTM, just had that one doubt

@jonaslagoni
Copy link
Member Author

@Souvikns anyone else need to review it or can we merge it?

@Souvikns
Copy link
Member

Yeah, you can merge, it is up to you.

@jonaslagoni
Copy link
Member Author

Not really up to me, it's your folks repo, I am just suggesting a change like any other contributor @Souvikns 😄

But if you say go ahead and merge then I wont stop 😄

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

some templates need to be added

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

lgtm

@Souvikns need another approval. Once you approve, just merge

@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

@Souvikns
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 94790a8 into asyncapi:master Jul 27, 2023
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.51.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@cykl cykl mentioned this pull request Jan 29, 2024
10 tasks
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.

4 participants