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

Fix redis:lookup regression (fixes #75) #76

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Conversation

myii
Copy link
Member

@myii myii commented Jan 29, 2019

I've already been submitting map.jinja PRs today:

Noticed issue #75 here was effectively the same issue re: lookup issues:

The issue here is that the lookup isn't being currently being merged in map.jinja.

The structure of this fix has been discussed in detail, particularly in the apache-formula pull.


I've tested before and after this PR, with the following lookup values, almost identical to pillar.example except for overcommit_memory, in order to test for #75:

  lookup:
    svc_state: running
    cfg_name: /etc/redis.conf
    pkg_name: redis-server
    svc_name: redis-server
    overcommit_memory: False

The diff of map.jinja rendering before and after this PR:

--- ad145b6 (before PR)
+++ 8b31c2c (after PR)
@@ -7,7 +7,7 @@
   "auto_aof_rewrite_percentage": 100,
   "bin": "/usr/local/bin/redis-server",
   "bind": "127.0.0.1",
-  "cfg_name": "/etc/redis/redis.conf",
+  "cfg_name": "/etc/redis.conf",
   "cfg_version": "3.2",
   "daemonize": "yes",
   "database_count": 16,
@@ -31,7 +31,7 @@
   "maxmemory_samples": 3,
   "no_appendfsync_on_rewrite": "no",
   "notify_keyspace_events": "",
-  "overcommit_memory": true,
+  "overcommit_memory": false,
   "pidfile": "/var/run/redis/redis-server.pid",
   "pkg_name": "redis-server",
   "port": 6379,
  • The same mapping is produced as before, now with the lookup being merged in as well.

Update: logging CI problems.

  • ubuntu-1804:
    • Failing with Job for redis-server.service failed because a timeout was exceeded.
  • centos-7:
    • Failing with No package redis-server available.
    • It's redis on centos-7, not redis-server.

@aboe76
Copy link
Member

aboe76 commented Jan 29, 2019

@myii thanks for the fix

@myii myii deleted the PR_fix-lookup-regression branch January 29, 2019 19:34
@myii
Copy link
Member Author

myii commented Jan 29, 2019

@aboe76 You're welcome, thanks for the review and merge.

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.

2 participants