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 'values' field to Tag Object #744

Closed
wants to merge 4 commits into from

Conversation

smoya
Copy link
Member

@smoya smoya commented Mar 28, 2022

Part of #654

This materializes an idea @maciej Urbańczyk brought on #654.
It is a RFC-0, so it can be treated as what it is. A suggestion.

This PR adds a new values field to Tag Object to declare a list of plain strings as values.

This is needed since the current Tag Object only defines a name that can be used as an identifier of the tag, but there is no other field for declaring values. Without this change, users are forced to compose tag names, i.e. name: "env:prod" or for declaring multiple values: name: "owner:platform,product

However, this penalize UX (and tooling one) since no format standard will be in place. Especially hard when displaying those tags in documentation, or when filtering by any of those tags. For example, filtering by the owner tag above.

Related issue(s):

#654

@smoya
Copy link
Member Author

smoya commented Mar 28, 2022

Examples added.

@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


```json
{
"name": "user",
"description": "User-related messages"
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason, there were tabs here.

@@ -1351,15 +1351,18 @@ Field Name | Type | Description
<a name="tagObjectName"></a>name | `string` | **Required.** The name of the tag.
<a name="tagObjectDescription"></a>description | `string` | A short description for the tag. [CommonMark syntax](https://spec.commonmark.org/) can be used for rich text representation.
<a name="tagObjectExternalDocs"></a>externalDocs | [External Documentation Object](#externalDocumentationObject) | Additional external documentation for this tag.
<a name="tagObjectValues"></a>values | [`string`] | The value(s) for the tag.
Copy link
Member

Choose a reason for hiding this comment

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

🤔 I'm not sure I like having values inside the global Tag object. We only verified this has a use case on servers but how is it going to affect the rest of the spec where Tag is used? Should we have another kind of Tag object just for servers?

Copy link
Member

@magicmatatjahu magicmatatjahu Mar 28, 2022

Choose a reason for hiding this comment

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

For other objects it will work the same way as for servers, just the tags will be treated as "metadata", with which you will be able to filter certain elements in the UI or via the CLI (of course we will have to implement this). Currently a tag only has a key (name field) but you can't define certain more specific values for that (we don't want use delimeter or something like). This can be compared to labels in Kubernetes where labels are used to filter/create relationships between resources - https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, I'm not sure if we should call it values or rather value in singular, even we allow having multiple. It is just a feeling I guess but the plural sounds weird to me in this case.

@dalelane
Copy link
Collaborator

I'm not sure what I think about this. I think my hesitation mainly comes from the fact that OpenAPI doesn't do this.

I know we're not beholden to what OAI do, and consistency for the sake of consistency isn't smart. But... tagging isn't a uniquely-event-centric thing, so I'm hesitant about diverging on things where it isn't specifically in service of asynchronous issues.

Like I say I'm not sure. If OAI didn't exist, purely looking at this idea in isolation, I'd probably be more in favour.

@smoya
Copy link
Member Author

smoya commented Mar 29, 2022

I'm not sure what I think about this. I think my hesitation mainly comes from the fact that OpenAPI doesn't do this.

I know we're not beholden to what OAI do, and consistency for the sake of consistency isn't smart. But... tagging isn't a uniquely-event-centric thing, so I'm hesitant about diverging on things where it isn't specifically in service of asynchronous issues.

Like I say I'm not sure. If OAI didn't exist, purely looking at this idea in isolation, I'd probably be more in favour.

I'm not sure what I think about this. I think my hesitation mainly comes from the fact that OpenAPI doesn't do this.

I know we're not beholden to what OAI do, and consistency for the sake of consistency isn't smart. But... tagging isn't a uniquely-event-centric thing, so I'm hesitant about diverging on things where it isn't specifically in service of asynchronous issues.

Like I say I'm not sure. If OAI didn't exist, purely looking at this idea in isolation, I'd probably be more in favour.

I understand your point. In fact, I didn't pay attention to how OAI does (now I realize we just ported it literally).
I like to keep things simple, and I agree this can introduce an extra layer of complexity. However, I see it as a nice UX improvement over what we have, especially for those composable tags.
Considering the examples in the Issue description and more in particular, the one @fmvilas showed in #654 (comment), I see composable tags are a needed thing.
Otherwise, as exposed in #654 (comment), some filters might be hard to implement on tooling (or too tricky).

As @magicmatatjahu mentioned, this pattern of key/value for tags is common in other solutions such as Kubernetes, where you can implement selectors that solve most of the issues of grouping and filtering objects (workloads in their case).

I see it as a UX improvement that allows more extensibility, even though not mandatory for any new feature yet afaik.

@fmvilas
Copy link
Member

fmvilas commented Mar 29, 2022

Yeah, that's a great point @dalelane. @smoya what about introducing the concept of labels and making them exactly the same as k8s labels? We can then recommend that labels are meant to be used when you want to filter data (like k8s do with selectors). And tags are still relevant when you simply want to "categorize" stuff. Not sure it makes sense, what do y'all think?

@smoya
Copy link
Member Author

smoya commented Mar 30, 2022

Yeah, that's a great point @dalelane. @smoya what about introducing the concept of labels and making them exactly the same as k8s labels? We can then recommend that labels are meant to be used when you want to filter data (like k8s do with selectors). And tags are still relevant when you simply want to "categorize" stuff. Not sure it makes sense, what do y'all think?

I wanted to make the same suggestion yesterday, however, those are the points that stopped me to do it:

  1. The term label and tag seems mostly the same.
  2. In consequence, It introduces ambiguity. When should I use a tag? When a label instead? What's the difference?
  3. Tooling will have to support filters or selectors for both types. I.e. in generators, the command for generating code for production servers would contain a selector/filter like env:production. Would the command expect the tag or the label? or both? we could fall back to the first thing we find (ie. label with key env and value production or fallback to tag with name env:production.

For the 1st and 2nd points, I would love to ask @dalelane about the terminology label vs tag (English is not my mother tongue).
Regarding the 3rd, makes me ask myself if we really need a new object (Label) or instead, use the one we have now, with or without the upgrade to have values (remember multiple values for the same tag would be difficult to implement without this upgrade). Because if we end up doing fallbacks, won't that mean both objects are made with the same purpose?

Are we at the starting point again now?😅

@fmvilas
Copy link
Member

fmvilas commented Mar 31, 2022

  1. The term label and tag seems mostly the same.
  2. In consequence, It introduces ambiguity. When should I use a tag? When a label instead? What's the difference?

Yeah, I actually searched for the difference between label and tag when replying above so fair point.

Are we at the starting point again now?😅

Pretty much yeah 😅 But that's fine 😄

So, my concern with changing the Tag Object is that people would not be able to reuse their definitions between OpenAPI and AsyncAPI. Actually, if we make the values field optional —and I think we should anyways—, the Tag Object would be portable from OpenAPI to AsyncAPI but not the other way around. But "portable" is not the same as "reusable", meaning that if someone has tags extracted into a separate file or registry, they would not be able to reference them from both places if we add a values field.

So my proposal is that we ping the OpenAPI folks and see if they would be happy to add the values field too. At least as a first step. If they don't want to add it and we don't agree with the reasons, we can always move forward from our side but we should always try to collaborate with the rest of specs as much as possible IMHO.

@derberg
Copy link
Member

derberg commented Apr 5, 2022

added comment here #654 (comment)

@asyncapi-bot asyncapi-bot deleted the branch asyncapi:2022-04-release April 27, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants