search

saltstack-formulas / zabbix-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
50 stars 119 forks source link

refactor(map and defaults): Update the map.jinja file and add yaml maps #134

Closed absmith82 closed 4 years ago

absmith82 commented 4 years ago

This changes the map.jinja file to be more like the standard template. It also adds defaults, osfamlymap, osfingermap, and osmap yaml files for easy mapping of the proper packages and settings for each OS and version of the OS.

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

myii commented 4 years ago

@absmith82 Thanks for starting this, it's a very useful PR. An initial comment: all of these files need to be moved to the zabbix directory, including replacing the current map.jinja. Also, this repo now uses semantic-release, so the commit messages need to be formatted accordingly. Please see the link to that from the README.

absmith82 commented 4 years ago

You are right,

Sorry I was careless and just uploaded the files to the wrong place after testing on my test server. I'll check the readme and reformat the commit message and fix the file structure.

Thanks

myii commented 4 years ago

@absmith82 I've run some tests to compare the map that is produced before and after this PR. The results need to be identical for a successful conversion. I've been able to fix things at my end, so I'll append a commit to this PR in a little while, so that you can look over the changes made. Once we're happy with that, we can squash the whole PR to fix the commit message, so that we get semantic-release working -- changing the title of the PR isn't enough, unfortunately.

absmith82 commented 4 years ago

Thanks for the update. I've been looking at the Travis ci tests and it looks like the tests are failing because of repo issues. But any updates to make it the same as before would be fine with me. I literally just the old map.jinja values and translated then into the different yaml map files I may have changed the postgres packages due to the old ones being virtual packages.

On Sat, Mar 21, 2020, 1:09 PM Imran Iqbal notifications@github.com wrote:

@absmith82 https://github.com/absmith82 I've run some tests to compare the map that is produced before and after this PR. The results need to be identical for a successful conversion. I've been able to fix things at my end, so I'll append a commit to this PR in a little while, so that you can look over the changes made. Once we're happy with that, we can squash the whole PR to fix the commit message, so that we get semantic-release working -- changing the title of the PR isn't enough, unfortunately.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/saltstack-formulas/zabbix-formula/pull/134#issuecomment-602089054, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJCDLA5K2CIIWU6R224KF3RIUGHLANCNFSM4LQVS52Q .

myii commented 4 years ago

Thanks for the update. I've been looking at the Travis ci tests and it looks like the tests are failing because of repo issues. But any updates to make it the same as before would be fine with me. I literally just the old map.jinja values and translated then into the different yaml map files I may have changed the postgres packages due to the old ones being virtual packages.

Issues

@absmith82 It's easiest to cover the situation by looking at the diffs. For all of the diffs, due to the nature of how I was generating the changes, anything with a minus (-) at the beginning is something which isn't quite right. It needs to be changed back to the corresponding line(s) with a plus (+), or shouldn't be there at all:

Resolution

See https://github.com/saltstack-formulas/zabbix-formula/compare/3d324d9...b7c0977 for the changes that have been added to this PR.

  1. Fixed map.jinja:
    1. Only the pillar lookup should be merged, not the whole pillar
    2. Used the latest structure from the template-formula
  2. Fixed osfamilymap.yaml:
    1. Added a space after the dash in each list entry
    2. Resolved minor formatting
  3. Modified osmap.yaml:
    1. Used the latest structure from the template-formula
  4. Fixed osfingermap.yaml:
    1. Removed the PostgreSQL entries for Ubuntu 18.04 and 16.04 -- these can be tackled in a subsequent PR, this PR should be a pure refactor only
    2. Used the latest structure from the template-formula

Result

All of the platforms are now passing:

Remaining steps

After confirming the changes, the commits need to be fixed so that semantic-release runs properly. The easiest way to do that is squashing the commits when merging this PR.

myii commented 4 years ago

@absmith82 So just to confirm, all of the platforms are passing for this PR:

The only thing remaining is the commitlint, so if you're OK with the changes that I've added, I'll squash and merge this PR, fixing the commit message at the same time.

absmith82 commented 4 years ago

I have no problems with the changes you made. Most were formatting mistakes I made so if you can squash the commit to fix the commitlint test that would be great.

myii commented 4 years ago

Thanks @absmith82, merged and released successfully:

myii commented 4 years ago

@absmith82 One more thing, you may want to send through a PR regarding the osfinger changes for Ubuntu 18.04 and 16.04. That can now be considered for inclusion as a new fix/feature.

myii commented 4 years ago

@absmith82 Just got reminded that #123 already contains the suggestion for updating all of the platforms to 3.0, so it might be worth working with that in mind for any new PR.