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

Ensure that all configs are reloaded after reset + setup #44

Merged
merged 2 commits into from
Apr 24, 2020

Conversation

smudge
Copy link
Member

@smudge smudge commented Apr 24, 2020

@nanda-prbot
Copy link

Needs somebody from @samandmoore and @effron to claim domain review
Needs somebody from @samandmoore and @effron to claim platform review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

HOW TO: Claim a Review

@samandmoore
Copy link
Member

I wonder if we can move the config loading after the check for enabled? It would be different from how things work right now but might be an improvement that avoids loading those files unless they need to be loaded? I can't think of anything that they'd need to be loaded for if setup is going to no op

@smudge
Copy link
Member Author

smudge commented Apr 24, 2020

I wonder if we can move the config loading after the check for enabled? It would be different from how things work right now but might be an improvement that avoids loading those files unless they need to be loaded?

This makes sense -- I went ahead and made the change. Not seeing anything break locally when I point apps to it.

def load_configs!
WebValve.config_paths.each do |root|
path = root.join('config', 'webvalve.rb').to_s
load path if File.exist?(path)
Copy link
Member

Choose a reason for hiding this comment

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

Don't love that we're doing ruby file io on every test case now, but we can look to optimize if it seems to actually be slow.

@samandmoore
Copy link
Member

Okay. Let's do this!

<<domainlgtm platformlgtm

@nanda-prbot
Copy link

Approved! 🎁 🙌 👌

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

Successfully merging this pull request may close these issues.

3 participants