Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor(map): use config.get instead of pillar.get #95

Merged
merged 1 commit into from
Apr 27, 2019

Conversation

myii
Copy link
Member

@myii myii commented Apr 24, 2019

We've had some discussions in Slack/IRC/Matrix about moving on from pillar.get to config.get. libtofs.jinja is already using the latter. Selected WIP for this PR since this is something to consider across all formulas. The intention here is to open up the discussion about going forward with this.

CC: @gtmanfred.

Copy link

@gtmanfred gtmanfred left a comment

Choose a reason for hiding this comment

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

i like this, because it allows pulling from grains as well as resolves sdb:// uris, so if someone wants to store the data in sdb, they can.

@daks
Copy link
Member

daks commented Apr 25, 2019

For now, I don't really see the use case for config.get instead of pillar.get. I read that it's to reduce load on busy masters, but I'm still not confronted to this problem.
And today all salt docs and 'litterature' (blog posts, ...) talk about pillar and none talk about anything else, like sdb. So I'm not sure if we already know what will replace pillars and there is no documentation.

In theory this has no impact because config.get also read pillars. But in reality I'm pretty sure that requet will take longer if all your data stays in pillars.

My 2 cents :)

in summary: I'm not opposed to this change even if this subject is still quite obscure for me, and I think we (and users above all) miss information about to do things differently.

@gtmanfred
Copy link

The time difference is negligible, since it just does a lookup for the key in the __grains__ __opts__ or __pillar__ object, and bails to the next one if it doesn't find it in the first.

Copy link
Contributor

@vutny vutny left a comment

Choose a reason for hiding this comment

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

I like config.get.

@daks
Copy link
Member

daks commented Apr 26, 2019

The time difference is negligible, since it just does a lookup for the key in the __grains__ __opts__ or __pillar__ object, and bails to the next one if it doesn't find it in the first.

Good to know @gtmanfred

@myii myii changed the title WIP: refactor(map): use config.get instead of pillar.get refactor(map): use config.get instead of pillar.get Apr 27, 2019
@myii myii merged commit 24bf7d0 into saltstack-formulas:master Apr 27, 2019
@myii
Copy link
Member Author

myii commented Apr 27, 2019

Thanks everyone for your feedback. I've merged this PR.

@myii myii deleted the refactor/map-config-get branch April 27, 2019 07:58
@saltstack-formulas-travis

🎉 This PR is included in version 2.0.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@myii
Copy link
Member Author

myii commented Apr 30, 2019

This commit may need to be reverted. config.get and pillar.get aren't 100% interchangeable, for example merge=True isn't available for the former. So if the result of the final config.get is an empty dict, the entire map ends up as an empty dict.

I've tried a workaround using defaults.merge again but I'm running into another problem:

TypeError: Cannot update using non-dict types in dictupdate.update()

Anyone know of how this can be resolved? Or is the only option to return back to pillar.get, for the time being at least?

@myii
Copy link
Member Author

myii commented Apr 30, 2019

Update: I've been able to find a solution to this by building upon the defaults.merge strategy we used to use earlier on. The problem with this is that this doesn't work for salt-ssh users. So the dilemma:

  1. Use config.get via. defaults.merge -- get the benefit but leave salt-ssh behind.
  2. Use pillar.get -- works for everyone but limited to pillar only.

myii added a commit to myii/template-formula that referenced this pull request May 1, 2019
* Fix 5dc0b86 in saltstack-formulas#95
  - No option `merge=True` for `config.get`
* Use `pillar.get` for `salt-call` (for `salt-ssh`)
* Use `config.get` otherwise
myii added a commit to myii/template-formula that referenced this pull request May 1, 2019
* Fix 5dc0b86 in saltstack-formulas#95
  - No option `merge=True` for `config.get`
* Use `pillar.get` for `salt-call` (i.e. `salt-ssh`)
* Use `config.get` via. `defaults.merge` otherwise
  - Reintroduce based on 775a930 in saltstack-formulas#20
myii added a commit to myii/template-formula that referenced this pull request May 6, 2019
* Fix 5dc0b86 in saltstack-formulas#95
  - No option `merge=True` for `config.get`
* Use `pillar.get` for `salt-call` (i.e. `salt-ssh`)
* Use `config.get` via. `defaults.merge` otherwise
  - Reintroduce based on 775a930 in saltstack-formulas#20
myii added a commit to myii/template-formula that referenced this pull request May 12, 2019
Co-Authored-By: Alexander Weidinger <aw@sz9i.net>

* Fix 5dc0b86 in saltstack-formulas#95
  - No option `merge=True` for `config.get`
* Use `pillar.get` for `salt-call` (i.e. `salt-ssh`)
* Use `config.get` otherwise
myii added a commit to myii/template-formula that referenced this pull request May 12, 2019
* Fix 5dc0b86 in saltstack-formulas#95
  - No option `merge=True` for `config.get`
* Use `pillar.get` for `salt-call` (i.e. `salt-ssh`)
  - Differentiate `salt-ssh`/`salt-call` via. `root_dir`
* Use `config.get` via. `defaults.merge` otherwise
  - Reintroduce based on 775a930 in saltstack-formulas#20
myii added a commit to myii/template-formula that referenced this pull request May 13, 2019
* Fix 5dc0b86 in saltstack-formulas#95
  - No option `merge=True` for `config.get`
* Use `pillar.get` for `salt-call` (i.e. `salt-ssh`)
  - Differentiate `salt-ssh`/`salt-call` via. `root_dir`
* Use `config.get` via. `defaults.merge` otherwise
  - Reintroduce based on 775a930 in saltstack-formulas#20
myii added a commit to myii/template-formula that referenced this pull request May 15, 2019
* `merge` not available via. `salt-ssh`
* Additionally, fix 5dc0b86 in saltstack-formulas#95
    - No option `merge=True` for `config.get`
saltstack-formulas-travis pushed a commit that referenced this pull request May 15, 2019
## [2.1.4](v2.1.3...v2.1.4) (2019-05-15)

### Bug Fixes

* **`map.jinja`:** remove `merge` from `config.get` (for `salt-ssh`) ([00e474c](00e474c)), closes [#95](#95)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants