Fix inconsistent $proxy_host handling in apt and apt::ppa. #330
Conversation
- The default for $proxy_host is undef - apt considers $proxy_set to be absent if $proxy_host is undef - apt::ppa considers proxy_env to be empty if $proxy_host is false or '' This results in apt::ppa to consider $proxy_host to be set when the default undef is used breaking ppa resources because $proxy_env becomes: [http_proxy=http://:8080, https_proxy=http://:8080] Fix this by making both apt and apt::ppa consider $proxy_host to be unset when it is any of false, '', or undef.
|
You've broken the tests because you haven't update the tests to the new behaviour. Until this is green we can't even consider this for merge, no matter how broken the current implementation is.
Comments like these aren't very helpful. If anything on a personal level I tend to turn away from contributions that try to pressure me into doing something because they need it 'right now'. |
|
Sorry, you're right I broke the tests. It was difficult but I managed to run the tests now. I didn't need to change the tests, I made a mistake in my implementation that broke the with proxy behaviour. I've fixed the mistake. If you see any others I'll fix them. And sorry for the pressure. But including this in a point/patch release after it's reviewed and merged would be appreciated since apt::ppa cannot be used on most hosts from the version in the forge since this bug cannot be worked around (using false or '' would result in 01proxy breaking instead). |
|
@mhaskel @apenney Could you guys look at this for a sec? It looks good to me but I'm not sure of the impact it might have. Would be nice to get that in for 1.5.1 though. |
|
Looks like 1.5.1 has been released first. Is there anything I can cleanup in this pull request? I notice now this repo has a |
|
@dantman our next planned release is 1.6.0, which should be coming out in the next couple of weeks, and this will definitely be included there. Would a 1.5.3 release with bugfixes backported be important to you? We could probably get that out next week if need be. If so, I'll handle the necessary cherry-picking. Thanks for the contribution! |
Fix inconsistent $proxy_host handling in apt and apt::ppa.
|
@mhaskel Sorry for not responding. Strictly speaking since this is a bug a point/bug release would be a good idea anyways. I do depend on some key PPAs for our main overseer/puppetmaster so whenever I make it replicate and bootstrap itself I have to go in and manually replace the puppetlabs/apt module with a git clone. |
|
@dantman we're going to try to get this rolled into a 1.5.3 release for next week. |
|
@dantman We released apt 1.6.0 today that contains a fix for this issue. Could you possibly upgrade to 1.6.0? |
|
@mhaskel ^_^ I don't have a version restriction for puppetlabs/apt right now, so with a new release my issue should already be fixed, thanks. |
This results in apt::ppa to consider $proxy_host to be set when the default undef is used
breaking ppa resources because $proxy_env becomes:
Fix this by making both apt and apt::ppa consider $proxy_host to be unset when it is any of false, '', or undef.
Please merge and release, apt::ppa is unusable as-is.