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

feat: Add base config override #4

Merged
merged 1 commit into from
May 15, 2019
Merged

feat: Add base config override #4

merged 1 commit into from
May 15, 2019

Conversation

YOU54F
Copy link
Contributor

@YOU54F YOU54F commented May 8, 2019

Hi,

Just wondered if you would be interested in a proposal to include the ability to custom configure the config_base for the app.

I have got this working with the pact_broker app, however I pull in the config from another repo, into a pact_broker folder, and rather than changing any references in that code, it would be handy to change the config base.

This is a bit of a hack for now, but it worked, happy to make it a bit cleaner, and more in link with existing rack custom vars if you would be happy to consider

PS, thank you for the great project and top docs!

Thanks!

@codecov
Copy link

codecov bot commented May 8, 2019

Codecov Report

Merging #4 into master will decrease coverage by 0.2%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #4      +/-   ##
==========================================
- Coverage     100%   99.79%   -0.21%     
==========================================
  Files           5        5              
  Lines         487      496       +9     
  Branches       60       64       +4     
==========================================
+ Hits          487      495       +8     
- Misses          0        1       +1
Impacted Files Coverage Δ
index.js 100% <100%> (ø) ⬆️
lib/rack_adapter.rb 90% <75%> (-10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbba7cf...7497849. Read the comment docs.

@logandk
Copy link
Owner

logandk commented May 9, 2019

Hi @YOU54F, thanks for the PR! It makes sense to make the Rack config path configurable. RACK_CONFIG_BASE isn't a standard Rack-thing, right? Because there's also the option of making this a setting in serverless.yml, which I would prefer for consistency - except if we have a standardized Rack-counterpart, in which case we should follow that convention.

@YOU54F
Copy link
Contributor Author

YOU54F commented May 9, 2019

Hey bud, no it’s not a standard rack env var. I will pop it into a custom rack var to follow the format of your other config options and add readme instructions

@logandk
Copy link
Owner

logandk commented May 9, 2019

Awesome 👍 The config gets written from here: https://github.com/logandk/serverless-rack/blob/master/index.js#L89

@YOU54F
Copy link
Contributor Author

YOU54F commented May 9, 2019

Hey mate,

I reckon this is good for review now,

I have tested it on this branch/PR in one of my github projects

pact-foundation/pact_broker-serverless#13

Running on a lambda here

https://you54f.co.uk/

Cheers!

@logandk
Copy link
Owner

logandk commented May 9, 2019

@YOU54F Great work, much appreciated! I'll review the changes and do a release as soon as possible

@YOU54F
Copy link
Contributor Author

YOU54F commented May 9, 2019

@YOU54F Great work, much appreciated! I'll review the changes and do a release as soon as possible

Brill, no rush my friend!

@logandk logandk merged commit 7497849 into logandk:master May 15, 2019
@logandk
Copy link
Owner

logandk commented May 15, 2019

It's merged now - I changed set setting name to configPath since it's basically just the Rack configuration file and not going to be inherited by some final configuration file - hope you agree. Again, thanks for your work!

@YOU54F
Copy link
Contributor Author

YOU54F commented May 15, 2019

That’s makes perfect sense mate, much appreciated. I’ll test it out later and report back just to close the loop.

Thanks again for the package, glad to help out :)

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