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

Jump to bottom

Support Central Config #464

Closed
wants to merge 6 commits into from
Closed

Support Central Config #464

wants to merge 6 commits into from

Conversation

mikker
Copy link
Contributor

@mikker mikker commented Jul 3, 2019 •
edited

Closes #446
Depends on #463

@mikker mikker self-assigned this Jul 3, 2019
houndci-bot
houndci-bot reviewed Jul 3, 2019
View reviewed changes
spec/elastic_apm/config_spec.rb Outdated Show resolved Hide resolved
spec/elastic_apm/config_spec.rb Outdated Show resolved Hide resolved
lib/elastic_apm/config/bytes.rb Outdated Show resolved Hide resolved
lib/elastic_apm/config/options.rb Outdated Show resolved Hide resolved
lib/elastic_apm/config/options.rb Outdated Show resolved Hide resolved
lib/elastic_apm/central_config/cache_control.rb Outdated Show resolved Hide resolved
lib/elastic_apm/central_config/cache_control.rb Show resolved Hide resolved
simitt
simitt reviewed Jul 4, 2019
View reviewed changes
lib/elastic_apm/central_config.rb Show resolved Hide resolved
lib/elastic_apm/central_config.rb Outdated Show resolved Hide resolved
lib/elastic_apm/central_config.rb Outdated Show resolved Hide resolved
lib/elastic_apm/central_config.rb Show resolved Hide resolved
lib/elastic_apm/central_config.rb Outdated Show resolved Hide resolved
lib/elastic_apm/central_config.rb Outdated Show resolved Hide resolved
@bmorelli25
Copy link
Member

👋Mikker. I'll be out of the office when you're back, but here's some boilerplate documentation you can hopefully adapt to match your config style: elastic/apm#86. Also, don't forget to update the ruby precedence. Would be great if it included a link to the new configuration variable.

@mikker mikker marked this pull request as ready for review August 1, 2019 10:59
axw
axw requested changes Aug 2, 2019
View reviewed changes
lib/elastic_apm/central_config.rb Outdated Show resolved Hide resolved
@mikker
Copy link
Contributor Author

mikker commented Aug 12, 2019

@simitt, @axw Will you give this a second, quick review?

@mikker
Copy link
Contributor Author

mikker commented Aug 12, 2019

Squashed all commits to hopefully make the tests run ....

axw
axw requested changes Aug 12, 2019
View reviewed changes
Copy link
Member

@axw axw 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, but I think there might be a problem with applying updates on 304. Could be misreading it though -- an extension of the 304 test would good either way.

lib/elastic_apm/central_config.rb Show resolved Hide resolved
lib/elastic_apm/central_config.rb Show resolved Hide resolved
spec/elastic_apm/central_config_spec.rb Show resolved Hide resolved
axw
axw approved these changes Aug 12, 2019
View reviewed changes
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! LGTM

lib/elastic_apm/central_config.rb Outdated Show resolved Hide resolved
@v1v
Copy link
Member

v1v commented Aug 14, 2019

jenkins run the tests please

@mikker mikker mentioned this pull request Aug 15, 2019
Merged
@mikker
Copy link
Contributor Author

mikker commented Aug 15, 2019

Continued in #484 because Jenkins

@mikker mikker closed this Aug 15, 2019
@mikker mikker deleted the central-config branch January 2, 2020 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simitt simitt simitt left review comments

@houndci-bot houndci-bot houndci-bot left review comments

@axw axw axw approved these changes

Assignees

@mikker mikker

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support remote configuration
7 participants
@mikker @bmorelli25 @v1v @axw @simitt @houndci-bot @lreuven