Proxy ensure parameter.

[mike-callahan]

Added an ensure parameter for user control of proxy presence. Defaults to undef to resist breaking any current setup.

[daenney]

I think I understand what you're trying to achieve here but could you list the use cases? The code isn't entirely obvious.

[mike-callahan]

Currently, once a proxy is set through the module it can't be removed. This will allow a user to remove a proxy they no longer want set.

[daenney]

Thanks. Unfortunately this is lacking tests right now for the new behaviours so if you could have a look at https://github.com/puppetlabs/puppetlabs-apt/blob/master/spec/classes/apt_spec.rb and add to them we can move forward.

[mike-callahan]

Done.

[daenney]

Right. All I need is a squash and we should be good to go as long as @mhaskel has no objections 😄.

[underscorgan]

I think my only comment is that we should probably validate we're getting a valid value for ensure.

[daenney]

That's a good point, validate_re($proxy['ensure'], ['file', 'present', 'absent', undef] and make sure to only check that if $proxy['ensure'].

[mike-callahan]

I didn't validate because it is already being validated at a lower level under apt::setting. I can still add if you'd like. By squash do you mean combining my commits?

[daenney]

Mmm that's true but validating it here allows us to give a slightly more descriptive error message since the user is not necessarily aware that it's being passed on to another type and as such might be confused about a resource suddenly throwing an error it has no idea about.

Yes, squash is turning your commits into 1. We prefer 1 feature to map to 1 commit and tests are included as part of accepting a new feature. It keeps the history clean and facilitates things like writing chaneglogs.

[mike-callahan]

Squashed and validated.

[underscorgan]

I don't think this is going to behave the way you want it to. You'll end up with a weird proxy file if you have ensure != absent and the proxy host isn't specified.

[mike-callahan]

How about:

if $proxy['ensure'] == 'absent' or $proxy['host'] {
    apt::setting { 'conf-proxy':
      ensure   => $_proxy['ensure'],
      priority => '01',
      content  => template('apt/_conf_header.erb', 'apt/proxy.erb'),
    }
  }

[underscorgan]

I think we'd need to use $_proxy['ensure'] instead of $proxy['ensure'] to make sure it doesn't break strict variables, but otherwise yes, I think that should work.

[daenney]

Yap, need to use $_proxy[] for consistency.

[mike-callahan]

Do you mean like this?

if $_proxy['ensure'] == 'absent' or $_proxy['host'] {
    apt::setting { 'conf-proxy':
      ensure   => $_proxy['ensure'],
      priority => '01',
      content  => template('apt/_conf_header.erb', 'apt/proxy.erb'),
    }
  }

[daenney]

Yes

[daenney]

It's actually a bug on our side right now,

puppetlabs-apt/manifests/init.pp

Lines 67 to 72 in cd37fc3

	if $proxy['host'] {
	apt::setting { 'conf-proxy':
	priority => '01',
	content => template('apt/_conf_header.erb', 'apt/proxy.erb'),
	}
	}

doesn't use $_proxy but it should have.

[mike-callahan]

Ok should be all set

[underscorgan]

👍 thanks for the work @callahm3 !

[mike-callahan]

Thanks!