search

saltstack-formulas / influxdb-formula

Installs and configures the InfluxDB timeseries database
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Apache License 2.0
9 stars 34 forks source link

feat(tofs): add support of TOFS #26

Closed n-rodriguez closed 4 years ago

myii commented 5 years ago

Initial feedback, I haven't looked at this in any detail at all. But in terms of yamllint, this will cause an error with Jinja tags in the YAML files. Ideally, we need to resolve this by pre or post-processing in map.jinja (or similar). Worst case, we can ignore the YAML files containing the Jinja, but that then loses the benefit of having yamllint.

n-rodriguez commented 5 years ago

@myii ping

myii commented 5 years ago

@n-rodriguez I'll check this soon, thanks for the reminder.

myii commented 5 years ago

@n-rodriguez Can you check all of the Travis failures? From the first couple, there's a common error:

       local:
           Data failed to compile:
       ----------
           Rendering SLS 'base:influxdb.package.install' failed: Jinja variable 'dict object' has no attribute 'toml_pkg'
n-rodriguez commented 5 years ago

@myii it seems that influxdb is not available on this OpenSuse version (leap15) : https://software.opensuse.org/package/influxdb

n-rodriguez commented 5 years ago

default-centos-6-2017-7-py2 doesn't have any python-pytoml package

n-rodriguez commented 5 years ago

default-amazonlinux-2-2019-2-py2 doesn't have any python-pytoml package

myii commented 5 years ago

@n-rodriguez Like last time, do you want to enable all of the instances, to see which ones work?

n-rodriguez commented 4 years ago

@n-rodriguez Like last time, do you want to enable all of the instances, to see which ones work?

done : https://travis-ci.com/saltstack-formulas/influxdb-formula/builds/126296726

myii commented 4 years ago

@n-rodriguez The opensuse instance was working prior to this (specifically default-opensuse-leap-15-2018-3-py2). Can we get that working again?

n-rodriguez commented 4 years ago

@myii ping

myii commented 4 years ago

@n-rodriguez OK, to get an idea of what this PR is doing, I've taken advantage of the new debug mode to compare runs:

In terms of TOFS specifically, there wasn't much to check. In fact, before and after aren't even the same states (when searching for source:):

https://travis-ci.org/myii/influxdb-formula/jobs/590033880#L1099-L1102 (before):

       influxdb_logrotate:
         file.managed:
           - name: /etc/logrotate.d/influxdb
           - source: salt://influxdb/files/logrotate.conf.jinja

https://travis-ci.org/myii/influxdb-formula/jobs/590038193#L991-L997 (after):

       influxdb/config/install:
         file.managed:
           - name: /etc/influxdb/influxdb.conf
           - source: 
             - salt://influxdb/files/1d1421e8f779/influxdb.tmpl.jinja
             - salt://influxdb/files/Debian/influxdb.tmpl.jinja
             - salt://influxdb/files/default/influxdb.tmpl.jinja

Just checked right now using the following search:

So it looks like I'm going to have to do both runs again, with more states defined in kitchen.yml. Let me get back with that feedback, when I get a chance to do that.

myii commented 4 years ago

@n-rodriguez So influxdb.cli has been removed? Then this PR involves a breaking change (which isn't an issue but) that needs to be added to the commit message.

myii commented 4 years ago

@n-rodriguez OK, I only needed to modify the run done before TOFS, to get the necessary comparisons:

This breakdown is part of the familiarisation, so if I ask questions, please don't take it the wrong way.

https://travis-ci.org/myii/influxdb-formula/jobs/590048079#L1047-L1050:

       influxdb_config:
         file.managed:
           - name: /etc/influxdb/config.toml
           - source: salt://influxdb/files/influxdb.config.jinja

https://travis-ci.org/myii/influxdb-formula/jobs/590048079#L1065-L1068:

       influxdb_default:
         file.managed:
           - name: /etc/default/influxdb
           - source: salt://influxdb/files/influxdb.etc_default

https://travis-ci.org/myii/influxdb-formula/jobs/590048079#L1162-L1165:

       influxdb_logrotate:
         file.managed:
           - name: /etc/logrotate.d/influxdb
           - source: salt://influxdb/files/logrotate.conf.jinja

If anything else is being removed by this PR, it will also need to be added as a breaking change. Multiple breaking changes can be supplied in the commit body, like has been done here:

sticky-note commented 1 year ago

@myii @n-rodriguez Why did you closed this ??