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

Use JSON import for environment variables #64

Merged
merged 2 commits into from
Jun 19, 2020

Conversation

clementd-fretlink
Copy link
Contributor

@clementd-fretlink clementd-fretlink commented Jun 18, 2020

clever env import now supports reading a JSON value, a list of {"name": "MY_VAR", "value": "yolo" } objects.

Using JSON instead of an ad-hoc format makes things more robust, especially wrt quotes and newlines. I have tested it on lane-explorer: https://gitlab.in.fretlink.com/fretlink/lane-explorer/-/jobs/156189

The tricky part is that the values need to be provided as strings (as are environment variables, after all). This was done a bit implicitly previously, now this is done explicitly (this also explains why i was not able to use the dict2items filter directly.

Warning

Before this PR, when we provide an env var containing line breaks, they are escaped and transformed into literal \n, that need to be unescaped later. As far as I know, it's only done for the SSH key in ci-commons. With this PR, this step is not useful anymore and can be removed.

@clementd-fretlink clementd-fretlink force-pushed the json-env branch 3 times, most recently from e6653b2 to 4af70a5 Compare June 19, 2020 08:12
The env can be provided as a JSON list `[{"name": "PORT", "value": "8080"}]`.
The `dict2items` filter provided by ansible is _almost_ what we want, but it
keeps the value original types (a boolean is kept as a boolean in the JSON value).
Since environment variables are strings and `clever-tools` does not want to make
the implicit coercion for us, we need to do it ourselves.
@clementd-fretlink clementd-fretlink changed the title [WIP] Use JSON import for environment variables Use JSON import for environment variables Jun 19, 2020
@clementd-fretlink clementd-fretlink marked this pull request as ready for review June 19, 2020 08:29
Copy link

@adfretlink adfretlink left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@paulrbr-fl paulrbr-fl left a comment

Choose a reason for hiding this comment

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

I'd rather have the variable manipulation done outside the template. Its usually better to understand the variable manipulation next to the ansible tasks.

@paulrbr-fl
Copy link
Contributor

Warning

Before this PR, when we provide an env var containing line breaks, they are escaped and transformed into literal \n, that need to be unescaped later. As far as I know, it's only done for the SSH key in ci-commons. With this PR, this step is not useful anymore and can b

I am sorry but I don't understand this warning. Where do we “escape and transform line breaks into literal \n”?

@clementd-fretlink
Copy link
Contributor Author

The escaping was done by the to_json filter here: https://github.com/fretlink/ansible-clever/blob/master/templates/env.j2#L2 in addition to adding quotes, it also escapes special chars

The unescaping is performed by echo -e here: https://github.com/fretlink/continuous-integration-commons/blob/master/dhall/deploy/ansible/clever/DefaultEnv.dhall#L68

tasks/environment.yml Outdated Show resolved Hide resolved
@paulrbr-fl
Copy link
Contributor

The escaping was done by the to_json filter here: https://github.com/fretlink/ansible-clever/blob/master/templates/env.j2#L2 in addition to adding quotes, it also escapes special chars

Hm, but that's by nature of JSON values. And we were already transforming any values to be a JSON value. So I still don't understand the difference before and after the PR on the values we provide to clever

@clementd-fretlink
Copy link
Contributor Author

The difference is that before this PR, we provide a file KEY=value, where value is a JSON literal.
clever-tools treats it as a regular string, so it does not remove the escaping. That's why in the case of the ssh key, we have to use echo -e to remove the escaping.

With this, PR, clever-tools reads the env as JSON values and performs the unescaping itself

@clementd-fretlink
Copy link
Contributor Author

Using to_json with the KEY=value format was a trick to properly escape quotes and newlines, but it's still a trick. Here, we use json to serialize the env, both on the ansible side and on the clever-tools side

@paulrbr-fl
Copy link
Contributor

The difference is that before this PR, we provide a file KEY=value, where value is a JSON literal.
clever-tools treats it as a regular string, so it does not remove the escaping. That's why in the case of the ssh key, we have to use echo -e to remove the escaping.

With this, PR, clever-tools reads the env as JSON values and performs the unescaping itself

🤯 indeed very well noted 👌 thanks for the explanation.

@clementd-fretlink
Copy link
Contributor Author

Alright! After yesterday i was a bit scared to modify things further, but now it works well, so 🚀

@clementd-fretlink clementd-fretlink merged commit 18c278e into fretlink:master Jun 19, 2020
@clementd-fretlink clementd-fretlink deleted the json-env branch June 19, 2020 09:42
paulrbr-fl added a commit that referenced this pull request Jun 19, 2020
This is a fix to an unfortunate bug introduced by #64 because we had a
default value set to `None` on the
`clever_haskell_entry_point`. Ansible considers `None` as a defined
value so the `is defined` condition doesn't match our need
paulrbr-fl added a commit that referenced this pull request Jun 19, 2020
This is a fix to an unfortunate bug introduced by #64 because we had a
default value set to `None` on the
`clever_haskell_entry_point`. Ansible considers `None` as a defined
value so the `is defined` condition doesn't match our need
paulrbr-fl added a commit that referenced this pull request Jun 19, 2020
This is a fix to an unfortunate bug introduced by #64 because we had a
default value set to `None` on the
`clever_haskell_entry_point`. Ansible considers `None` as a defined
value so the `is defined` condition doesn't match our need
paulrbr-fl added a commit that referenced this pull request Jun 19, 2020
This is a fix to an unfortunate bug introduced by #64 because we had a
default value set to `None` on the
`clever_haskell_entry_point`. Ansible considers `None` as a defined
value so the `is defined` condition doesn't match our need
paulrbr-fl added a commit that referenced this pull request Jun 19, 2020
This is a fix to an unfortunate bug introduced by #64 because we had a
default value set to `None` on the
`clever_haskell_entry_point`. Ansible considers `None` as a defined
value so the `is defined` condition doesn't match our need
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.

3 participants