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

Support server URL variables #182

Closed
fmvilas opened this issue Nov 22, 2019 · 4 comments · Fixed by #227
Closed

Support server URL variables #182

fmvilas opened this issue Nov 22, 2019 · 4 comments · Fixed by #227
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@fmvilas
Copy link
Member

fmvilas commented Nov 22, 2019

Currently, the generator doesn't support server URL variables. For instance, when we have a server like this:

servers:
  production:
    url: test.mosquitto.org:{port}
    protocol: mqtt
    description: Test broker
    variables:
      port:
        description: Secure connection (TLS) is available through port 8883.
        default: '1883'
        enum:
          - '1883'
          - '8883'

The generated code tries to connect to mqtt://test.mosquitto.org:{port}. It would be great if it could replace port by a generated value, i.e., the default value or a valid one, based on the variable definition.

@fmvilas fmvilas added enhancement New feature or request help wanted Extra attention is needed labels Nov 22, 2019
@fmvilas fmvilas mentioned this issue Dec 2, 2019
@derberg
Copy link
Member

derberg commented Feb 17, 2020

@fmvilas hey, can you share some more explanation here, I'm not fully getting what can be done here. My first attempt would be to replace {port} with default, but then looking at https://www.asyncapi.com/docs/specifications/2.0.0/#a-name-servervariableobject-a-server-variable-object I learn default is not required, so I would say, fallback to default is the first item from enum list. It could also be parametrized, and the user could maybe do -p variable.port.default.

Or...maybe this url: test.mosquitto.org:{port} should be explicit? and say url: test.mosquitto.org:{port.default}?

@fmvilas
Copy link
Member Author

fmvilas commented Feb 19, 2020

I think it should go this way:

  1. Check if there's a default value: if so, use it. Otherwise, continue to 2.
  2. Check if there's an enum. If so, get the first one. Otherwise, continue to 3.
  3. Do nothing, as it is right now. Maybe log a warning message would be great, but anything else.

say url: test.mosquitto.org:{port.default}?

{port.default} is a variable itself. Different from {port}. I'd not introduce syntax/expressions on the variable names just because it would be great to have them on the generator. Let's be cautious with making the spec a config file for our tools.

What do you think?

@derberg
Copy link
Member

derberg commented Feb 19, 2020

I actually did not take into account that port.default could be an individual variable. You're right.

So default -> 1st enum -> warning

2 more questions:

  • what templates are we talking here about? all? only the code related ones, right? for docs you do not want such magic
  • is parser already warning the user that he has a variable (in url like in example) that is not defined under variable parameter?

I'm also thinking where later describe such functionality, so the user knows how it works without reading the templating. Or maybe it is enough to log clearly, that in case of default, log will say default was used, in case of enum, log will say first enum was used?

@fmvilas
Copy link
Member Author

fmvilas commented Feb 19, 2020

what templates are we talking here about? all? only the code related ones, right? for docs you do not want such magic

Correct. That's only for code generation templates. So far, I'd only target the Node.js template.

is parser already warning the user that he has a variable (in url like in example) that is not defined under variable parameter?

Nope, no warnings yet. This would be a great use case for stoplightio/spectral#965. Actually, I'd forget about triggering warnings in the generator for now. Just go for: default -> 1st enum -> nothing (like now).

If we're going to log warnings we should probably make them customizable, and that's a Spectral ruleset in the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants