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

remove check, if $release is empty #78

Merged
merged 1 commit into from
Aug 22, 2012

Conversation

saz
Copy link

@saz saz commented Jul 3, 2012

At the moment, it's not possible to add sources, with an empty release value.

For example: http://pkg.jenkins-ci.org/debian/

This pull request removes the check, if $release is empty.

@branan
Copy link

branan commented Jul 3, 2012

I don't think this will quite do what you want. In cases where the lsbdistcodename fact is available it will still try to use that as the release value. I believe the module will need a bit more surgery in order to safely support sources without a release.

@saz
Copy link
Author

saz commented Jul 3, 2012

With this change, I'm able to set release => '' or release => undef which is enough to do what I want.

@saz
Copy link
Author

saz commented Jul 3, 2012

FYI: I've only tried it with release => ''.

@branan
Copy link

branan commented Jul 3, 2012

OK, that makes sense.

I was just talking with @RColeman about this, and keeping that warning about lsbdistcodename is something we want so that the default behavior can still be relied upon safely. We also want to support your use case, of course.

I think the way to do this is to have a default release = 'UNDEF' and do additional logic in the module to verify lsbdistcodename is sane only in the case that the default value is used. If release is specified, it would bypass the check for lsbdistcodename.

If you want to do the extra work for the implementation that would be really awesome, otherwise we can open up an issue on redmine for your use case so that it stays on our radar.

@saz
Copy link
Author

saz commented Jul 3, 2012

I think this should work. At least it looks like as it is doing what it should :)

@branan
Copy link

branan commented Jul 3, 2012

Looks good :)

A couple more nitpicks:

  • Please rebase so your previous commit/revert don't get merged into the main history. It's just noise
  • Please add a test for the new functionality. I probably wouldn't care about this on a different module, but Apt is practically our poster-child for good test coverage and I'd like to not let that slide.

bodepd added a commit that referenced this pull request Aug 22, 2012
remove check, if $release is empty
@bodepd bodepd merged commit 32d312e into puppetlabs:master Aug 22, 2012
@LukasAud LukasAud added the bugfix label Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants