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

stack liquid templating makes a man drink: IF branch or ELSE branch doesn't get executed #1890

Closed
matti opened this issue Feb 23, 2017 · 11 comments

Comments

@matti
Copy link
Contributor

matti commented Feb 23, 2017

See this one from

    # {% if lb_service %}
    ports:
      - 8086:8086
    # {% else %}
    ports:
      - 8086:8086
    # {% endif %}

below:

stack: matti/influxdb
version: 0.0.1
description: base
expose: api

variables:
  image:
    type: string
    default: influxdb:1.2-alpine
    from:
      prompt:
  affinity:
    type: string
    default: label!=no-influxdb
    from:
      prompt:
  lb_service:
    type: string
    required: false
    from:
      service_link:
        prompt: Pick a loadbalancer
        image: kontena/lb
  lb_domain:
    only_if: lb_service
    type: string
    default: influxdb.example.net
    from:
      prompt:
  lb_https_redirect:
    only_if: lb_service
    type: boolean
    from:
      prompt:

services:
  api:
    image: {{ image }}
    affinity:
      - {{ affinity }}
    stateful: true
    deploy:
      strategy: ha
      wait_for_port: 8086
      interval: 1d
    volumes:
      - /var/lib/influxdb
    environment:
    # {% if lb_service %}
      - KONTENA_LB_INTERNAL_PORT=8086
      - KONTENA_LB_VIRTUAL_HOSTS={{ lb_domain }}
      # {% if lb_https_redirect %}
      - KONTENA_LB_CUSTOM_SETTINGS=redirect scheme https if !{ ssl_fc }
      # {% endif %}
    # {% endif %}

    # {% if lb_service %}
    ports:
      - 1234:8086
    # {% else %}
    ports:
      - 4321:8086
    # {% endif %}

    links:
    # {% if lb_service %}
      - {{ lb_service }}
    # {% endif %}
@matti
Copy link
Contributor Author

matti commented Feb 23, 2017

@kke ?

@matti
Copy link
Contributor Author

matti commented Feb 23, 2017

So this always results in: 8086/tcp

CONTAINER ID        IMAGE                       COMMAND                  CREATED             STATUS              PORTS                                         NAMES
78fdfb0f8606        influxdb:1.2-alpine         "/w/w /entrypoint...."   14 seconds ago      Up 13 seconds       8086/tcp                                      influxdb.api-1

@kke
Copy link
Contributor

kke commented Feb 23, 2017

Both of them seem to have the port 8086? :)

@matti
Copy link
Contributor Author

matti commented Feb 23, 2017

... that wasn't the point: they don't get mapped. edited it now.

@kke
Copy link
Contributor

kke commented Feb 23, 2017

Have you tried

ports:
  # {% if lb_service %}
  - 1234:8086
  # {% else %}
  - 4567:8086
  # {% endif %}

?

@SpComb
Copy link
Contributor

SpComb commented Feb 23, 2017

Odd, with kontena stack validate, if I select an lb_service, then it does render the ports correctly, but selecting <none> to omit the optional lb_service, it does not render either of the if-else options:

stack: matti/influxdb
version: 0.0.1
description: base
expose: api
variables:
  image:
    label: image
    type: string
    default: influxdb:1.2-alpine
    from:
      prompt: 
      env: image
    to:
      env: image
    errors: {}
    value: influxdb:1.2-alpine
  affinity:
    label: affinity
    type: string
    default: label!=no-influxdb
    from:
      prompt: 
      env: affinity
    to:
      env: affinity
    errors: {}
    value: label!=no-influxdb
  lb_service:
    label: lb_service
    type: string
    from:
      service_link:
        prompt: Pick a loadbalancer
        image: kontena/lb
      env: lb_service
    to:
      env: lb_service
    required: false
    errors: {}
    value: 
  lb_domain:
    label: lb_domain
    type: string
    default: influxdb.example.net
    from:
      prompt: 
      env: lb_domain
    to:
      env: lb_domain
    only_if: lb_service
    errors: {}
    value: 
  lb_https_redirect:
    label: lb_https_redirect
    type: boolean
    from:
      prompt: 
      env: lb_https_redirect
    to:
      env: lb_https_redirect
    only_if: lb_service
    errors: {}
    value: 
  STACK:
    label: STACK
    type: string
    from:
      env: STACK
    to:
      env: STACK
    errors: {}
    value: influxdb
  GRID:
    label: GRID
    type: string
    from:
      env: GRID
    to:
      env: GRID
    errors: {}
    value: development
services:
  api:
    image: influxdb:1.2-alpine
    affinity:
    - label!=no-influxdb
    stateful: true
    deploy:
      strategy: ha
      wait_for_port: 8086
      interval: 1d
    volumes:
    - "/var/lib/influxdb"
    environment: 
    links: 

@matti
Copy link
Contributor Author

matti commented Feb 23, 2017

yes. and also verified that this "works" aka port can be mapped:

    # {% if lb_service %}
    ports:
      - 8086:8086
    # {% else %}
    ports:
      - 8086:8086
    # {% endif %}

    # this will map
    ports:
      - 8086:8086

@SpComb
Copy link
Contributor

SpComb commented Feb 23, 2017

With #1884 that case raises an error:

> Pick a loadbalancer <none>
> Enter image : influxdb:1.2-alpine
> Enter affinity : label!=no-influxdb
 [error] Liquid error: undefined variable lb_service

I assume what's happening is that the optional lb_service variable is not defined, and Liquid::Template.render(.., strict_variables: true) fails with an error. That error is just ignored in the current CLI, and it continues with the rest of the template, omitting the entire if-else-endif block.

The optional variables should be defined as nil instead...

@matti
Copy link
Contributor Author

matti commented Feb 23, 2017

while at it also fix #1888 for double 🍻 rewards

@matti
Copy link
Contributor Author

matti commented Feb 24, 2017

same bug here?

$ kontena stack install matti/weavescope

> Pick a loadbalancer lb-ingress/lb
> Enter image : weaveworks/scope:0.17.1
> Enter lb_domain : weavescope.lol.com
> Enable lb_https_redirect : ****no****
 [done] Creating stack weavescope
 [done] Triggering deployment of stack weavescope
 [done] Waiting for deployment to start
 [done] Deploying service app
 [done] Deploying service probe
$ kontena service show weavescope/app
[clipped]
  env:
    - KONTENA_LB_INTERNAL_PORT=4040
    - KONTENA_LB_VIRTUAL_HOSTS=weavescope.lol.com
    - ******KONTENA_LB_CUSTOM_SETTINGS=redirect scheme https if !{ ssl_fc }*****

SpComb added a commit that referenced this issue Mar 1, 2017
The current CLI stacks YAML reader uses `Liquid::Template.render(..., strict_variables: true)`. This means that that any references to undefined variables will omit the tag from the output and push an error message into `template.errors`. The CLI ignores those errors, which cascades into other difficult to debug issues when parts are missing from the resulting YAML:

* #1846
* #1890

A Liquid bug also means that optional `required: false` stack variables also behave as undefined variables: Shopify/liquid#749

This PR fixes the CLI stack YAML reader to:

* Workaround Liquid bugs to treat optional  `required: false` stack variables as falsey values for Liquid conditional blocks

* Fail `kontena stack ...` fast with a  `Liquid error: undefined variable ... ` message if the YAML contains invalid variable references.

    These errors would be nicer if they also contained the source YAML `file:line`, but that is missing from the `Liquid::UndefinedVariable` error...
@SpComb
Copy link
Contributor

SpComb commented Mar 1, 2017

Fixed in #1884 - seems I left the magic out of the commit message

@SpComb SpComb closed this as completed Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants