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

Support for pre.Dockerfile.* #3999

Merged
merged 5 commits into from Jul 16, 2022
Merged

Support for pre.Dockerfile.* #3999

merged 5 commits into from Jul 16, 2022

Conversation

hanoii
Copy link
Collaborator

@hanoii hanoii commented Jul 15, 2022

Partially reverts

From Discord: https://discord.com/channels/664580571770388500/997163304822771842

Release Consequences

@github-actions
Copy link

github-actions bot commented Jul 15, 2022

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Just took a quick look before bed, looks like it's probably fine. Will look more closely tomorrow. The docs problem is just a silly markdown rule. Thanks!

@rfay
Copy link
Member

rfay commented Jul 15, 2022

Oh, there is a very specific reason for the explicit proxy support - docker-compose doesn't actually put the HTTP_PROXY into the build stage at this point, meaning that you can't build images or add to images that require proxying without detecting the HTTP_PROXY as was done in that code. It's unfortunate but true. We have to figure out how to get HTTP_PROXY into the build stage.

@hanoii
Copy link
Collaborator Author

hanoii commented Jul 15, 2022

Oh, there is a very specific reason for the explicit proxy support - docker-compose doesn't actually put the HTTP_PROXY into the build stage at this point, meaning that you can't build images or add to images that require proxying without detecting the HTTP_PROXY as was done in that code. It's unfortunate but true. We have to figure out how to get HTTP_PROXY into the build stage.

I don't think this is correct. First, the only thing your code seems to be doing is checking for the presence of the env var and if so, making it available on the Dockerfile through ENV and then doing the APT part. The checking is not necessary and you can just add it to your Dockerfile without that. To second this we have the actual use case of #3960, the fact that we was able to make it work on 1.19.2 means that you don't really need the code you added to make it work. Am I missing something here?

@hanoii
Copy link
Collaborator Author

hanoii commented Jul 15, 2022

Fixed the md error.

@hanoii
Copy link
Collaborator Author

hanoii commented Jul 15, 2022

Hmm, it still didn't pass the docs check but I see no error in the job, and I can't seem to be able to trigger a re-run.

EDIT: I saw the error, a link not working but it must have been a glitch because the one reported is definitely up and valid.

@rfay
Copy link
Member

rfay commented Jul 15, 2022

Don't worry about that docs check. Sometimes random sites start blocking various kinds of access, we'll sort it out. Obviously not the fault of this PR.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I'll manually test this and we can talk about next steps wrt proxy.

@@ -879,21 +879,6 @@ func WriteBuildDockerfile(fullpath string, userDockerfilePath string, extraPacka
ARG BASE_IMAGE
FROM $BASE_IMAGE
`
// Provide proxy handling inside container if necessary
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to say I jumped the gun about being able to remove this. Not sure if there's been a change in docker-compose v2 or what, but docker-compose v2 does not respect HTTP_PROXY (or pass it in) at build time, meaning that if we don't inject this into the Dockerfile it doesn't get there. It does get there without this for docker run but not for docker-compose build, meaning that behind a proxy adding extra debian packages doesn't work without something like this approach. I can't say when this changed, but I was unable to get webimage_extra_packages to work (ever) without injecting the HTTP_PROXY into the build instructions as here.

Copy link
Collaborator Author

@hanoii hanoii Jul 15, 2022

Choose a reason for hiding this comment

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

But how did the OP on #3960 made it work before on v1.19.2? I am missing something in regards of env vars.

I gave something a try with my local build:

a pre.Dockerfile.proxy with:

ARG HTTP_PROXY
# The below env file can be removed if you don't want it to also be an env var. I would probably add it though.
ENV HTTP_PROXY=$HTTP_PROXY
RUN echo $HTTP_PROXY > /tmp/proxy

And a

docker-compose.proxy.yml with:

version: '3.6'

services:
  web:
    build:
      args:
        - HTTP_PROXY=${HTTP_PROXY}

worked for me, I had the content of my manually set environment var HTTP_PROXY in /tmp/proxy on the container as well as an env var set.

@rfay
Copy link
Member

rfay commented Jul 15, 2022

Yes, your approach with an arg would work as well, should be the same thing. And I guess a ddev-get add-on that added something to the web-image build args would solve it. So yes, that's got potential. I'll have to test it.

@rfay
Copy link
Member

rfay commented Jul 16, 2022

Works with https://github.com/rfay/ddev-proxy-support - would love anybody else to try that out. You need the artifacts from #3999 (comment) (or build this PR) and then ddev get rfay/ddev-proxy-support - You'll need at least HTTP_PROXY set in your environment.

@rfay
Copy link
Member

rfay commented Jul 16, 2022

Rebased and added capabilities marker in ddev debug capabilities

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Looks great to me, tests great. Thanks! And thanks for the collaboration. I am so very happy not to be putting explicit proxy support in here.

@rfay rfay merged commit 0abc6ff into ddev:master Jul 16, 2022
torenware pushed a commit to torenware/ddev that referenced this pull request Jul 18, 2022
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.

None yet

2 participants