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

feat(config): salt has a module to serialize to TOML #4

Merged
merged 5 commits into from
Mar 10, 2020

Conversation

baby-gnu
Copy link
Contributor

@baby-gnu baby-gnu commented Mar 6, 2020

The use of a custom module is not required anymore since salt 2017 support ended 2019-12-31.

  • telegraf/files/default/telegraf.tmpl.jinja: replace custom module call with salt['slsutil.serialize'].

BREAKING CHANGE: salt['slsutil.serialize'] requires salt >= 2018.3


@baby-gnu
Copy link
Contributor Author

baby-gnu commented Mar 6, 2020

It looks like on Centos, the salt-minion does not find the newly installed toml python3 library.

For openSUSE, this is an infrastructure problem.

@baby-gnu
Copy link
Contributor Author

baby-gnu commented Mar 6, 2020

It looks like on Centos, the salt-minion does not find the newly installed toml python3 library.

I found the issue in map.jinja, the python3-toml package was installed for the builtin module but Centos 8 has a python3-pytoml.

Copy link
Member

@n-rodriguez n-rodriguez 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
Member

@myii myii left a comment

Choose a reason for hiding this comment

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

@baby-gnu Nice work, just a minor change requested for now.

telegraf/map.jinja Outdated Show resolved Hide resolved
@myii
Copy link
Member

myii commented Mar 7, 2020

Bit short on time but a few more comments:


Thanks for the review, @n-rodriguez.

@myii
Copy link
Member

myii commented Mar 7, 2020

  • toml serializer was introduced in 2018.3

@baby-gnu It would be worth adding this as a BREAKING CHANGE message as well. Two reasons why the minimum version supported is 2018.3.

The use of a custom module is not required anymore since salt 2017
support ended 2019-12-31.

* telegraf/files/default/telegraf.tmpl.jinja: replace custom module
  call with `salt['slsutil.serialize']`.

BREAKING CHANGE: `salt['slsutil.serialize']` was introduced in `2018.3`
BREAKING CHANGE: `toml` serializer was introduced in `2018.3`
@baby-gnu
Copy link
Contributor Author

baby-gnu commented Mar 9, 2020

I think they should be managed in a separate pull request with a rework of the map.jinja and the handling of the python library.

@baby-gnu
Copy link
Contributor Author

baby-gnu commented Mar 9, 2020

We should note that maybe someday the toml library used will change : saltstack/salt#56323

@myii
Copy link
Member

myii commented Mar 10, 2020

@baby-gnu Would you mind including this commit to finalise this PR?

I tried to push this on directly on top of this PR but didn't have the permissions to do so. It appears that Allow edits from maintainers is disabled.

@baby-gnu
Copy link
Contributor Author

Note that there is no python toml library on amazonlinux

./bin/kitchen exec default-amazonlinux-2-master-py3 -c 'sudo yum search toml'
-----> Execute command on default-amazonlinux-2-master-py3.
       Failed to set locale, defaulting to C
       Modules compl?mentaires charg?s?: ovl, priorities
       Attention : aucune correspondance trouvée pour : toml
       No matches found

@myii myii merged commit 75e29e5 into saltstack-formulas:master Mar 10, 2020
@myii
Copy link
Member

myii commented Mar 10, 2020

@baby-gnu Thanks for adding the commit.

Note that there is no python toml library on amazonlinux

Actually, there is a package available by adding the EPEL repo:

I'm sure it would be possible to get amazonlinux working by adding this repo. There are examples of doing this inline in other formulas, which I could dig out if required.

@saltstack-formulas-travis

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@myii
Copy link
Member

myii commented Mar 11, 2020

@baby-gnu Re: amazonlinux-2, similar to the change made to the openvpn-formula.

@baby-gnu
Copy link
Contributor Author

Thanks @myii, as I'm starting to use the telegraf-formula, I think I can work a little bit more on it. But amazonlinux is not my priority for now.

@baby-gnu baby-gnu deleted the feature/drop-custom-module branch March 11, 2020 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants