ci: preserve PATH in run_envoy_docker.sh#18467
ci: preserve PATH in run_envoy_docker.sh#18467bartsmykla wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
By using `-E` flag with sudo we so far were preserving env vars in start command. The problem is it's not preserving PATH var, which is fine for ubuntu based images as it looks like all tools are available within default PATH (/sbin:/bin:/usr/sbin:/usr/bin). The problem starts when we need to modify this PATH, (like in centos images: https://github.com/envoyproxy/envoy-build-tools/blob/55a7bbe700586729bd38231a9a6f3dcd1ff85e7d/build_container/Dockerfile-centos#L21) This change is explicitly passing the PATH variable from the container the start command is being called Signed-off-by: Bart Smykla <bartek@smykla.com>
|
Hi @bartsmykla, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
@bartsmykla im ambivalent about updating the centos image as i think it is causing confusion, is not tested anywhere, and doesnt get updates/attention that the main build image gets nevertheless, i think their may be an argument for keeping it for the purpose of building for older glibc versions im happy to help looking at what changes are required to make it work, at least for that specific use case. at very least before landing any code i would like to see it working. if we can make it work, im also thinking that we should include it in CI somewhere, so it continues to do so wrt this PR im thinking that any updates/changes to the in the case of the centos image it is already set in the image, so any changes should be there, i think |
|
@phlax thank you for your response, I actually made it work, and all the fixes are present here: envoyproxy/envoy-build-tools#154 You are right that all changes/requirements related to os specific |
|
@bartsmykla i see re sudo so then my next thought is why is it working for ubuntu and not centos the ubuntu image sets and uses PATH, so its not really clear what the difference would be - i can do some testing on this to figure out re testing the final product - i would like to see it tested in some kind of CI, even if its temporary |
|
@phlax looking inside the container it doesn't seem so: https://github.com/envoyproxy/envoy-build-tools/blob/main/build_container/Dockerfile-ubuntu I can of course move the PATH from Dockerfile to the script if it's what you prefer About testing this in CI, I can look into https://github.com/envoyproxy/envoy-proxy-tools and I can try write some test, but I'm no expert on Azure Pipelines by no mean |
|
I will be also very happy to help maintaining CentOS images/tools |
im happy to add a testing pr/branch with azp set up |
|
@phlax ok, so I'll update envoyproxy/envoy-build-tools#154 and will move |
|
@phlax done |
|
ok, give me a bit of time to look at this further, i dont use the build images directly myself so i need to fetch them etc |
|
no worries, thank you for your help |
2 participants
Commit Message:
By using
-Eflag with sudo we so far were preserving env varsin start command. The problem is it's not preserving PATH var,
which is fine for ubuntu based images as it looks like all tools
are available within default PATH (/sbin:/bin:/usr/sbin:/usr/bin).
The problem starts when we need to modify this PATH, (like in
centos image)
This change is explicitly passing the PATH variable from
the container the start command is being called
Additional Description:
none
Risk Level:
I don't see any potential risks
Testing:
I have built envoy using both, centos and ubuntu images
Docs Changes:
no changes
Release Notes:
no release notes
Platform Specific Features:
it let's envoy-build-centos image to work again