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 tags field to Server Object #809

Merged
merged 7 commits into from
Sep 22, 2022

Conversation

smoya
Copy link
Member

@smoya smoya commented Jun 17, 2022

Description

This PR adds support for defining Tags at Server Object. They can be used for providing metadata, including the environment such as production, development, etc.

Related issue(s)
Fixes #654
Fixes #465

@smoya smoya changed the base branch from next-spec to master June 17, 2022 15:59
@smoya smoya changed the base branch from master to next-spec June 17, 2022 16:02
@smoya smoya mentioned this pull request Jun 17, 2022
spec/asyncapi.md Outdated
@@ -361,6 +361,7 @@ Field Name | Type | Description
<a name="serverObjectDescription"></a>description | `string` | An optional string describing the host designated by the URL. [CommonMark syntax](https://spec.commonmark.org/) MAY be used for rich text representation.
<a name="serverObjectVariables"></a>variables | Map[`string`, [Server Variable Object](#serverVariableObject)] | A map between a variable name and its value. The value is used for substitution in the server's URL template.
<a name="serverObjectSecurity"></a>security | [[Security Requirement Object](#securityRequirementObject)] | A declaration of which security mechanisms can be used with this server. The list of values includes alternative security requirement objects that can be used. Only one of the security requirement objects need to be satisfied to authorize a connection or operation.
<a name="serverObjectTags"></a>tags | [Tags Object](#tagsObject) | A list of tags for API documentation control. Tags can be used for logical grouping of servers.
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid using API and documentation. These are just specific use cases IMHO. What about this:

Suggested change
<a name="serverObjectTags"></a>tags | [Tags Object](#tagsObject) | A list of tags for API documentation control. Tags can be used for logical grouping of servers.
<a name="serverObjectTags"></a>tags | [Tags Object](#tagsObject) | A list of tags for logical grouping and categorization of servers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shall we apply this change to Operation, OperationTrait, Message, and MessageTrait as well? I.e. https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md?plain=1#L712

Copy link
Member

Choose a reason for hiding this comment

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

That would be awesome yes 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Okey changes applied.

However, cc @alequetzalli in case you want to take this opportunity to add, remove, fix, and or rephrase this description.

@smoya smoya requested a review from fmvilas July 11, 2022 13:17
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.

I would be good to extend some of the official examples, like streetlights for kafka with this feature, as this use case is most common, that Kafka has multiple production servers. Could make sense to also add example description in Tags object.

separate question:
I think we need to mention at least in this PR that we are aware there is a feature that allows you too specify that some channel is available only on a given server (CHANNEL_NAME.servers).

I somehow feel that people will immediately assume, they can refer not only to a server by server name but the tag too 😄

@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2022

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
No Duplication information No Duplication information

@smoya
Copy link
Member Author

smoya commented Jul 20, 2022

I would be good to extend some of the official examples, like streetlights for kafka with this feature, as this use case is most common, that Kafka has multiple production servers.

I don't fully get what you mean by Kafka has multiple production servers. Would you mind sharing an example or clarifying it? 🙏

Could make sense to also add example description in Tags object.

Good suggestion! I've pushed a new commit with the requested change.

separate question: I think we need to mention at least in this PR that we are aware there is a feature that allows you too specify that some channel is available only on a given server (CHANNEL_NAME.servers).

I somehow feel that people will immediately assume, they can refer not only to a server by server name but the tag too 😄

TBH I don't see the relation between CHANNEL_NAME.servers and the tags. Those are not selectors nor similar.

@smoya smoya requested review from derberg and dalelane July 20, 2022 15:16
@derberg
Copy link
Member

derberg commented Jul 25, 2022

I don't fully get what you mean by Kafka has multiple production servers. Would you mind sharing an example or clarifying it? 🙏

so with Kafka you can have a cluster of brokers. Related discussion #465

I'm pretty sure people will use tag to address above.

TBH I don't see the relation between CHANNEL_NAME.servers and the tags. Those are not selectors nor similar.

Am I really the only one (with Kafka cluster use case in mind) that thinks that people will like to do CHANNEL_NAME.servers=['env:production'] 😄

@fmvilas
Copy link
Member

fmvilas commented Jul 25, 2022

Am I really the only one (with Kafka cluster use case in mind) that thinks that people will like to do CHANNEL_NAME.servers=['env:production'] 😄

This is a really good point 🤔 Maybe something to consider for 3.0.0?

@char0n char0n mentioned this pull request Sep 20, 2022
61 tasks
@smoya
Copy link
Member Author

smoya commented Sep 20, 2022

@fmvilas @dalelane @derberg a last review so we can include this into 2.5 finally?

@derberg
Copy link
Member

derberg commented Sep 20, 2022

is this PR resolving #654? I think yes, so please update description

do we agree that this PR also solves #465? 👀 @dalelane that wanted to champion the other proposal


@smoya 👇🏼 was not yet addressed

I would be good to extend some of the official examples, like streetlights for kafka with this feature, as this use case is most common, that Kafka has multiple production servers

and regarding 👇🏼

TBH I don't see the relation between CHANNEL_NAME.servers and the tags. Those are not selectors nor similar.

I think we have an agreement that for 3.0 we want to go $ref-everywhere direction, including CHANNEL_NAME.servers, so I guess we can just leave it as it is without any concerns.

@smoya
Copy link
Member Author

smoya commented Sep 21, 2022

@smoya 👇🏼 was not yet addressed

@derberg I updated the socialmedia, streetlights-kafka and streetlights-mqtt examples. Can you take a look to those? 🙏

@smoya smoya requested review from derberg and removed request for asyncapi-bot-eve September 21, 2022 14:16
@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2022

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

@derberg
Copy link
Member

derberg commented Sep 21, 2022

@dalelane can you have a final look please, ref -> #809 (comment)

Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

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

This looks really good - I like the examples, as they are super helpful prompts for the ways that tags can be used, instead of just mechanically showing how to add a tag 👍

@derberg
Copy link
Member

derberg commented Sep 22, 2022

@dalelane thanks, does it also solve #465 from your perspective?

@dalelane
Copy link
Collaborator

@dalelane thanks, does it also solve #465 from your perspective?

yeah I think so - I'll add a comment there, too

@derberg
Copy link
Member

derberg commented Sep 22, 2022

@smoya can you update the description accordingly so the other issue is also resolved after merge, by default?

and then merge 💪🏼

@derberg
Copy link
Member

derberg commented Sep 22, 2022

@magicmatatjahu something we might want to support in react component out of the box after release 😉

@smoya
Copy link
Member Author

smoya commented Sep 22, 2022

@smoya can you update the description accordingly so the other issue is also resolved after merge, by default?

and then merge 💪🏼

Done at the same time you posted this comment ⚡ 🏃

@magicmatatjahu
Copy link
Member

@derberg I will remember :)

@char0n
Copy link
Collaborator

char0n commented Sep 22, 2022

Looks all good here and ready for 2.5.0, going for a merge. Thanks @smoya!

@derberg
Copy link
Member

derberg commented Sep 22, 2022

@char0n
Copy link
Collaborator

char0n commented Sep 22, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit b0c2f9c into asyncapi:next-spec Sep 22, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.5.0-next-spec.2 🎉

The release is available on GitHub release

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.

None yet

7 participants