-
Notifications
You must be signed in to change notification settings - Fork 26
CIDM-1566 - Add via-header to container config #84
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
CIDM-1566 - Add via-header to container config #84
Conversation
0727242
to
df75755
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes don't really reflect how this option changed. You can find more details here: http://www.openrepose.org/versions/8.10.0.0/architecture/container.html#via-header-configuration
Sadly, we have failed to provide an example there, but you can find one here: https://github.com/rackerlabs/repose/blob/master/repose-aggregator/tests/functional-tests/src/integrationTest/configs/features/core/via/element/container.cfg.xml
I'd probably also add a check to make sure that via and the new options aren't ever trying to be used simultaneously, because that will fail within repose.
df75755
to
c8f1013
Compare
@adrianjgeorge All of your comments should be addressed in this amended commit.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any deal breakers here.
c8f1013
to
b6d6c6e
Compare
@adrianjgeorge The two issues you mentioned have been resolved in the amended commit. Thanks for the catches. |
Looks good to me. |
With the new behavior surrounding the via, via-header and repose-version options in container config. We found we were leaking the repose version. This updates the puppet module so we can control the via header and repose version in the user response.