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

Implement channel name regex configuration #1458

Merged
merged 1 commit into from
Apr 7, 2021
Merged

Implement channel name regex configuration #1458

merged 1 commit into from
Apr 7, 2021

Conversation

vvilerio
Copy link

Hi,
this PR allows externalizing the channel formalisation regex found in add-channel.component.html.
The regex will be in the DEFAULT_CONFIG of the config.service.ts file and editable with the VALIDATION_CHANNELNAME_REGEXP_ENV environment variable.

However, it must be taken into account that this regex will cross the different build layers.

Hi,
this PR allows externalizing the channel formalisation regex found in add-channel.component.html.
The regex will be in the DEFAULT_CONFIG of the config.service.ts file and editable with the VALIDATION_CHANNELNAME_REGEXP_ENV environment variable.

However, it must be taken into account that this regex will cross the different build layers.
@EricWittmann
Copy link
Member

@lbroudoux and @carlesarnal what do you think? If this makes sense, should we be doing it for all of the other name validation regex as well?

@GoulvenF
Copy link

Hi,

For your information, we need this feature to be able to integrate some "business" rules on the channels namings. To be more precise, in our corp, the ACL's on the Kafka broker are derivated from conventions on channel names.

This feature would allow us to integrate this specific business requirement in a completely configurable way on the Apicurio Studio.

Imho, having it on all other naming validations could be nice, but it is probable than ACL's conventions (or other) rely more on channels name/format than on other contracts convention.

To think ahead of validation features, I wonder if integrating Apicurio with an external validation component (linter, such as Prism) would not be the more accurate way to handle this kind of mechanism. Would be happy to have @EricWittmann's point of view on this consideration. If it makes sense, we would be happy to engage some developments on this kind of feature.

Best regards,

@carlesarnal
Copy link
Member

@lbroudoux and @carlesarnal what do you think? If this makes sense, should we be doing it for all of the other name validation regex as well?

Yes, I think it makes sense to make all the regex configurable like this one.

@lbroudoux
Copy link

lbroudoux commented Mar 29, 2021

Hi!

Whilst I completely understanding the requirement, I'm wondering if it's the best place to put this configuration. IMHO I tend to think that at the editor level we should stick with the regexp that is coming from the AsyncAPI specification. I tend to think that additional business requirements and conventions should be checked using custom validation rules. That said I'm not sure if we currently allow loading custom validation rules in the studio...

@GoulvenF : regarding this specific use case of security settings on Kafka binding, I think you should have a look at this previous proposition on AsyncAPI bindings : asyncapi/bindings#56. If it's related to protocol or broker configuration, it should land within the binding spec IMHO.

What do you think?

@GoulvenF
Copy link

GoulvenF commented Apr 1, 2021

Hi,

@lbroudoux I understand the intention to be the most respectful of the asyncapi spec, in that way the intent to be fully compliant with the asyncapi regexp is a good point. However, should it not be the default behaviour (or configuration)? I guess that some organisations (like us) will have to deal with some specific use cases that could be leveraged with the possibility to specialize those regexes.

On the implementation of our specific use case, thanks for the link. I had a look at asyncapi/bindings#56, but I think we are not on the same considerations. Indeed, we are not on protocols concerns (which have to be described in the bindings) , but on programatically derivated rules from the channel name. (ACLs between publishers and subscribers on the kafka broker are computed from channel names, then applied on the kafka broker with a third party component)

Best regards,

@EricWittmann
Copy link
Member

Some observations on this from me. The first is that I think overall, custom validation rules make sense for this type of thing. I agree with @lbroudoux on that point. Unfortunately we do not yet have that feature in Studio (though it's been on the wish list for awhile). Also note that a validation rule would only apply after a change has been made, it would be some sort of additional feature to map UI form input validation to validation rules, so that a change would not be allowed to begin with it would violate a rule (custom or otherwise). It's not a bad idea, of course, but harder to define.

@EricWittmann
Copy link
Member

So it seems to me that this might be a reasonable (if short term) approach. My concern is that if we have this feature here, it would probably be expected everywhere (other modal dialogs that allow user-provided entity names).

@GoulvenF
Copy link

GoulvenF commented Apr 6, 2021

Hi,

I agree with the "reasonable if short term approach". We will have a look on the way validation rules works, and the ways to extend them, with the (stalled ?) #818 as a starting point. Please, feel free to provide any hints or directions.

Best regards,

@EricWittmann
Copy link
Member

I agree with the "reasonable if short term approach". We will have a look on the way validation rules works, and the ways to extend them, with the (stalled ?) #818 as a starting point. Please, feel free to provide any hints or directions.

OK I think we have agreement. I'm inclined to merge this as a temporary solution (it's a light touch). For now, custom validation rules are not supported - you are right that #818 is stalled. I'm certainly open to suggestions. The way that the apicurio-data-models library is built and used limits the options (to some degree). But that's not why it's stalled - that has more to do with competing priorities. :)

@EricWittmann EricWittmann merged commit 965c22e into Apicurio:master Apr 7, 2021
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.

5 participants