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

add opts to repo name #31

Merged
merged 5 commits into from
Jan 9, 2018

Conversation

arthurzenika
Copy link
Contributor

No description provided.

@@ -30,6 +30,7 @@ debian-archive-keyring:
{%- set r_arch = '[arch=' ~ args.arch|join(',') ~ ']' if args.arch is defined else '' %}
{%- set r_url = args.url or default_url %}
{%- set r_distro = args.distro or 'stable' %}
{%- set r_opts = args.opts if args.opts is defined else '' %}
Copy link
Member

Choose a reason for hiding this comment

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

As I understand from the manpage, the format is

deb [arch=i386 trusted=yes another=whatever] http://deb.debian.org/debian stable contrib non-free main

and not

deb [arch=i386] [trusted=yes another=whatever] http://deb.debian.org/debian stable contrib non-free main

so I think that we should merge r_arch and r_opts if the latter is set. Perhaps we can turn it a hash parameter, like

...
opts:
  trusted: 'yes'
  another: whatever
...
{% set r_opts = '' %}

{%- set r_arch = 'arch=' ~ args.arch|join(',') if args.arch is defined else '' %}
{% if args.opts is defined %}
  {% if args.opts is string %}
    {% set r_opts = args.opts %}
  {% else %}
    {%- for k, v in args.opts.items() %}
      {% set r_opts = r_opts ~ ' ' ~ k ~ '=' ~ v %}
    {%- endfor %}
  {% endif %}
{% endif %}

{% if r_arch != '' or r_opts ! = '' %}
  {% set r_options = '[' ~ r_arch ~ ' ' ~ r_opts ~ ']'
{% else %}
  {% set r_options = ''
{% endif %}
...
...
  pkgrepo.managed:
    - name: {{ r_type }} {{ r_options }} {{ r_url }} {{ r_distro }} {{ r_comps }}

what do you think? Haven't tested this, is just a 'before my 1st. coffee' idea, so maybe it's wrong 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@javierbertoli looks good, I hadn't seen that bit of the documentation but you're absolutely right. I'm going to try out your code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appart from small typos, it works for the string pillar, but not the dict pillar (the for k, v in args.opts.items() doesn't seem to work. Is it a jinja local variable problem ?

contributed initialy by @javierbertoli <javier@netmanagers.com.ar>

fixes saltstack-formulas#30
@aboe76 aboe76 requested a review from javierbertoli January 4, 2018 21:15
Copy link
Member

@javierbertoli javierbertoli left a comment

Choose a reason for hiding this comment

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

Other than a little change, it looks good to me.

{% else %}
{% set r_opts_list = [] %}
{%- for k, v in args.opts.items() %}
{% do r_opts_list.append(' ' ~ k ~ '=' ~ v) %}
Copy link
Member

Choose a reason for hiding this comment

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

As you're converting the hash into a list, you can skip the initial whitespace here, so it'd be

 {% do r_opts_list.append(k ~ '=' ~ v) %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks

@javierbertoli javierbertoli merged commit 47adb61 into saltstack-formulas:master Jan 9, 2018
@javierbertoli
Copy link
Member

Thanks, @arthurlogilab!! Great work!

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.

2 participants