-
Notifications
You must be signed in to change notification settings - Fork 43
docs: document proxy support #414
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
Conversation
Still thinking about an integration test for this, to ensure that it always works. I'm happy to do that as a second PR. Keen to hear from reviewers though. |
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.
Nice work! Just a couple of nits. I especially like the demonstration you provided, it really aids understanding.
Separate PR is fine 👍 but always a fan of an integration test for this sort of functionality. |
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.
Nice work on this documentation! There are a few things that I found that I think can be improved, but there's two thoughts I'd like to leave you with that I think can be beneficial to your writing in the future:
- Can this be said in less words, i.e. is this critical to get the point across?
- Can this be made more concrete?
Co-authored-by: Mathias Fredriksson <[email protected]>
Co-authored-by: Mathias Fredriksson <[email protected]>
Co-authored-by: Mathias Fredriksson <[email protected]>
Co-authored-by: Mathias Fredriksson <[email protected]>
Co-authored-by: Mathias Fredriksson <[email protected]>
Co-authored-by: Mathias Fredriksson <[email protected]>
Co-authored-by: Mathias Fredriksson <[email protected]>
Co-authored-by: Mathias Fredriksson <[email protected]>
Co-authored-by: Mathias Fredriksson <[email protected]>
@mafredri I've applied all of your review notes. I'm going ahead and merging this. If you would like to provide further feedback, I will open another PR to update :) Thanks for the review. |
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.
Great work on the rewrite, looks good. Spotted a stray deletion without compensation tho 😄
As before, this command yields a shell inside an Envbuilder built environment. Feel free to test it and then exit the container. Assuming this worked, Envbuilder will have cloned a repository and built the relevant container using a proxy that required accepting a custom CA certificate. | ||
|
||
### Bonus | ||
To prove that Envbuilder did in fact use the proxy, and also because it is interesting to observe, open `http://localhost:8081/` in your local browser and you see the mitmproxy web interface. In the flow tab, there be a list of all of the HTTP requests that were required to build the container. The first few requests will be those used to clone the Git repository. The rest will be the requests that were used to pull the devcontainer image. |
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.
To prove that Envbuilder did in fact use the proxy, and also because it is interesting to observe, open `http://localhost:8081/` in your local browser and you see the mitmproxy web interface. In the flow tab, there be a list of all of the HTTP requests that were required to build the container. The first few requests will be those used to clone the Git repository. The rest will be the requests that were used to pull the devcontainer image. | |
To prove that Envbuilder did in fact use the proxy, and also because it is interesting to observe, open `http://localhost:8081/` in your local browser and you see the mitmproxy web interface. In the flow tab, there will be a list of all of the HTTP requests that were required to build the container. The first few requests will be those used to clone the Git repository. The rest will be the requests that were used to pull the devcontainer image. |
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.
hotfixing
This PR closes #411