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

chore: add CODEOWNERS #138

Merged
merged 1 commit into from
Dec 13, 2021
Merged

chore: add CODEOWNERS #138

merged 1 commit into from
Dec 13, 2021

Conversation

derberg
Copy link
Member

@derberg derberg commented Dec 9, 2021

@fmvilas I wasn't sure here so I took an approach that JSON Schema is very closely aligned with spec then it should have exactly the same owners as spec repo.

We can rethink once we make a final decision about #130

Thoughts?

@derberg derberg requested a review from fmvilas December 9, 2021 18:27
@sonarcloud
Copy link

sonarcloud bot commented Dec 9, 2021

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

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.

Sounds good. My only suggestion is to add @jonaslagoni because of this: #128. I know it's not yet merged but that solves a lot of bugs and improves the repo by a lot.


# The default owners are automatically added as reviewers when you open a pull request unless different owners are specified in the file.

* @fmvilas @derberg @github-actions[bot]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @fmvilas @derberg @github-actions[bot]
* @fmvilas @derberg @jonaslagoni @github-actions[bot]

Copy link
Member Author

Choose a reason for hiding this comment

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

but then we are not aligned with spec repo 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

so maybe let us get #130 and #128 done first and then we can add more folks, huh?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm happy to wait but I'd not worry about aligning with spec. JSON Schemas are not the spec but a partial representation of it. If we align here we'd have to align on parsers too, which I don't think makes sense. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

so I know that we treat JSON Schema as a tool, but I was thinking about the alignment because it almost in 90% reflects the spec, not like in case of parser, and parser anyway depends on JSON Schema again in 90% for the validation.

if spec owners are not only gatekeepers for JSON Schema, in theory, it may happen that some validation rule can be changed here but not in spec. You know what I mean?

🤔

Copy link
Member

Choose a reason for hiding this comment

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

if spec owners are not only gatekeepers for JSON Schema, in theory, it may happen that some validation rule can be changed here but not in spec. You know what I mean?

Yeah, I get it, I just don't think it's very different than a parser implementing a custom validation that is not defined in the spec (like in asyncapi/spec#650). Anyway, I'm not going to block this PR just because of that.

I'm approving and we can consider adding other people later 👍

@derberg derberg merged commit 745f018 into asyncapi:master Dec 13, 2021
@derberg derberg deleted the addcode branch December 13, 2021 08:06
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.13.0-2022-01-release.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants