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

dokku_config: allow list type #55

Open
decentral1se opened this issue Mar 10, 2020 · 7 comments
Open

dokku_config: allow list type #55

decentral1se opened this issue Mar 10, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@decentral1se
Copy link
Member

decentral1se commented Mar 10, 2020

TASK [Configure the environment] *************************************************************************************************************************************************************************
fatal: [gitea]: FAILED! => {"changed": false, "meta": {"error": "All values must be keys, found invalid types for WHITELISTED_URIS", "present": false}, "msg": "All values must be keys, found invalid types for WHITELISTED_URIS"}

https://docs.gitea.io/en-us/config-cheat-sheet/

WHITELISTED_URIS: : If non-empty, list of POSIX regex patterns matching OpenID URI’s to permit.

So, I need to add the following config:

    - name: config env
      dokku_config:
        app: gitea
        config:
          WHITELISTED_URIS: ["foo.com"]

Shall we widen up the types that are accepted?

@decentral1se
Copy link
Member Author

decentral1se commented Mar 10, 2020

Actually, also "Foo: true" fails as well, which is pretty typical in an Ansible play. Looks like https://github.com/dokku/ansible-dokku/blob/master/library/dokku_config.py#L93 is the issue here. I think a bool should be easy to allow also. I imagine "FOO: 12" will also break then.

@josegonzalez
Copy link
Member

The error should say All values must be strings.

@josegonzalez
Copy link
Member

Description here as to my reasoning.

@decentral1se
Copy link
Member Author

Hmmm, Ansible does the following regarding trying to convert bools:

def to_bool(a):
    ''' return a bool for the arg '''
    if a is None or isinstance(a, bool):
        return a
    if isinstance(a, string_types):
        a = a.lower()
    if a in ('yes', 'on', '1', 'true', 1):
        return True
    return False

So, we could do that to address you concern in #6 (comment). We can extend the "in" check to cover other values that you might use / expect for truthiness.

I understand why it works the way it works now but it unfortunately breaks a very common expectation when writing Ansible plays that you can use integers, bools and not quote booleans. That is confusing coming from a strictly Ansible side of things.

@josegonzalez
Copy link
Member

The problem here is that folks won't be able to specify the difference between True (string) and True (bool) values, so trying to set those as values may result in issues at the application level. For instance, you might set true expecting that as the literal string, but then it gets set as the literal True string when we call config:set. The underlying Dokku command does not distinguish between value types, so attempting to support custom value types doesn't make sense insofar as I don't want to publish rules on how we serialize things.

The rule that you can use different types within Ansible only applies to when the input expects that. We do not, and the module expects a string value. Trying to coerce the value here doesn't make sense at all since you're attempting to set a literal value in your app's environment.

@decentral1se
Copy link
Member Author

Makes sense! I'll get a docs patch in to avoid this being raised up again 🌥️

@ltalirz ltalirz added the enhancement New feature or request label Sep 4, 2021
@ltalirz
Copy link
Member

ltalirz commented May 25, 2022

@decentral1se Is the docs patch in? Can we close this?

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

No branches or pull requests

3 participants