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.

Sign up for GitHub

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

Jump to bottom

Require dalli before using it #17269

Merged
merged 1 commit into from
Apr 10, 2018
Merged

Require dalli before using it #17269

merged 1 commit into from
Apr 10, 2018

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Apr 9, 2018

https://bugzilla.redhat.com/show_bug.cgi?id=1564647

Note, we require dalli in the session_store.rb only if you're configured
for session_store: cache. If you specify, 'sql', dalli doesn't get
required, and these two code paths could hit a NameError.

This was originally added in #15471

@jrafanie
Require dalli before using it
52e525d 
https://bugzilla.redhat.com/show_bug.cgi?id=1564647

Note, we require dalli in the session_store.rb only if you're configured
for session_store: cache.  If you specify, 'sql', dalli doesn't get
required, and these two code paths could hit a NameError.
@jrafanie jrafanie requested a review from gtanzillo April 9, 2018 21:38
@jrafanie
Copy link
Member Author

jrafanie commented Apr 9, 2018

@gtanzillo perhaps we should use the dalli store from rails?

manageiq/lib/token_store.rb

Line 14 in 142129e

ActiveSupport::Cache::DalliStore.new(MiqMemcached.server_address, cache_store_options(namespace, token_ttl))
cc @abellotti

@miq-bot
Copy link
Member

miq-bot commented Apr 9, 2018

Checked commit jrafanie@52e525d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@bdunne bdunne merged commit 7921773 into ManageIQ:master Apr 10, 2018
@bdunne bdunne assigned bdunne and unassigned gtanzillo Apr 10, 2018
@bdunne bdunne added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 10, 2018
@jrafanie jrafanie deleted the require_dalli_before_using_it branch April 10, 2018 14:17
simaishi pushed a commit that referenced this pull request May 14, 2018
@bdunne @simaishi
Merge pull request #17269 from jrafanie/require_dalli_before_using_it
ef82e93 
Require dalli before using it
(cherry picked from commit 7921773)

https://bugzilla.redhat.com/show_bug.cgi?id=1578126
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit ef82e931b8542136ed477e3bd3f57ea8fc3c145e
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Tue Apr 10 09:26:11 2018 -0400

    Merge pull request #17269 from jrafanie/require_dalli_before_using_it
    
    Require dalli before using it
    (cherry picked from commit 7921773f93d3c6514c97c2e10ae26b6ddf036906)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1578126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdunne bdunne bdunne approved these changes

@gtanzillo gtanzillo Awaiting requested review from gtanzillo

Assignees

@bdunne bdunne

Labels
blocker bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jrafanie @miq-bot @simaishi @bdunne @gtanzillo