Closed Smeb closed 4 years ago
rnorth
commented
4 years ago Hi @Smeb, thanks!
It's interesting - we use this with all sorts of build images (including ones derived from adoptopenjdk) and haven't encountered this problem.
Still, I think it's sensible to do this, and I can't see huge downsides with blanket-blacklisting these particular values.
I'll just suggest one small change inline.
Smeb
commented
4 years ago I've added your patch and fixed the indentation, thanks for the quick response!
If you're interested, the specific image we're basing off of is:
adoptopenjdk/openjdk8:jdk8u252-b09-debian-slim, which has the following path by default (which we don't really change):
PATH=/opt/java/openjdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
Smeb
commented
4 years ago @rnorth - is there anything further I need to do on this PR before we can merge it?
👋 hey, thanks for the plugin! We've found it very useful.
At the moment it has the somewhat tricky behavior of overriding the inner container's PATH and SHELL. We found this breaks openjdk images, for example. This code just blacklists environment variables which are already set by the image.
One downside of this would be if you foresee people intentionally overwriting the PATH in their drone secrets. If you think that's a problem we could try parsing to see if the variables changed.