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

Issue #618 : Accessing s3 bucket over a proxy from oc-registry #619

Merged
merged 3 commits into from
Sep 3, 2017
Merged

Issue #618 : Accessing s3 bucket over a proxy from oc-registry #619

merged 3 commits into from
Sep 3, 2017

Conversation

JuniorDev4Lyf
Copy link
Contributor

agentProxy property defined under the registry configuration object needs to be considered while establishing a connection to S3 Bucket from oc-registry.

This will allow the user to host the oc registry over an infrastructure behind a proxy.

Thanks


module.exports = function(conf) {
const httpOptions = { timeout: conf.s3.timeout || 10000 };
if (conf.s3.agentProxy) {
httpOptions.agent = proxy(conf.s3.agentProxy);
Copy link
Member

@matteofigus matteofigus Sep 1, 2017

Choose a reason for hiding this comment

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

If the property would be called conf.s3.proxy I would imagine it would be consistent to assign to the httpOptions.agent the proxy(url) value, but given we called it agentProxy I thought it was part of the idea to pass the whole agent already?

I think this would be a bit better given the naming, and also to allow who's not interested to use the proxy to avoid installing the proxy-agent dependency.

What's your thoughts on that @mattiaerre @NapoleanReigns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely agree with it. We shouldn't mandate the proxy-agent dependency to the user unless he is in need of it.

I will make the changes and submit soon.

@JuniorDev4Lyf
Copy link
Contributor Author

I have incorporated the review comments and pushed the changes.

Copy link
Member

@matteofigus matteofigus left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! I'll update the wiki.

@matteofigus matteofigus merged commit cff82dd into opencomponents:master Sep 3, 2017
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