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

Add options for rails_cache_store #762

Merged
merged 1 commit into from
May 3, 2020
Merged

Conversation

dgoetz
Copy link
Member

@dgoetz dgoetz commented Oct 16, 2019

This allows to make use of redis as backend, but still defaults to file.

@timogoebel: Can you have a look as the feature was implemented by you and so you perhaps know best if the parameters make sense in this way?

@dgoetz
Copy link
Member Author

dgoetz commented Oct 16, 2019

I think the failed tests are unrelated, because it is just Debian failing for "install foreman with prometheus" which runs fine on CentOS.

Copy link
Member

@timogoebel timogoebel left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

If the cache type is redis, it should install the foreman-redis package.

@dgoetz
Copy link
Member Author

dgoetz commented Oct 16, 2019

@ekohl I have it included now (also in the tests). Tests are still running.

manifests/init.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
@laugmanuel
Copy link
Member

@dgoetz can I support you in any way on getting this merged? I would love to experiment with this in our release environment.

@dgoetz
Copy link
Member Author

dgoetz commented Dec 17, 2019

@laugmanuel: No, it needs re-review and comments from @ekohl and @ehelms, so I get input for further adjustments if required.

@dgoetz dgoetz force-pushed the add_redis branch 2 times, most recently from 864aca8 to 042b281 Compare January 31, 2020 12:05
@dgoetz
Copy link
Member Author

dgoetz commented Jan 31, 2020

Rebased to resolve merge conflicts, tests failing on debian for statsd seem to be unrelated.

manifests/install.pp Outdated Show resolved Hide resolved
manifests/params.pp Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dgoetz
Copy link
Member Author

dgoetz commented Mar 11, 2020

I rebased and addressed the comments.

  • I removed the additional lines from the rebase.
  • An example was added.
  • I tested how a hash missing a key is handled and it works fine (according to the puppet docs it is simple handled as undef).

@ehelms
Copy link
Member

ehelms commented Apr 28, 2020

@dgoetz This will need another rebase

@dgoetz
Copy link
Member Author

dgoetz commented Apr 28, 2020

Rebased and tests are green except from beaker tests for puppet 5 testing journald, statsd and prometheus, not sure if this is related.

manifests/init.pp Outdated Show resolved Hide resolved
This allows to make use of redis as backend, but still defaults to file.
@dgoetz
Copy link
Member Author

dgoetz commented Apr 28, 2020

Tests are green now, and I hope I have addressed everything.

@ehelms ehelms requested a review from ekohl May 1, 2020 01:20
@ekohl ekohl merged commit 4307abe into theforeman:master May 3, 2020
@ekohl
Copy link
Member

ekohl commented May 3, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants