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

(#13125) Apt keys should be case insensitive #34

Merged
merged 1 commit into from
Mar 15, 2012
Merged

(#13125) Apt keys should be case insensitive #34

merged 1 commit into from
Mar 15, 2012

Conversation

blkperl
Copy link

@blkperl blkperl commented Mar 14, 2012

Previously lowercase keys would be installed every
puppet run because apt-key list returns an uppercase
key. This commit makes the comparison case insensitive.

@@ -20,18 +22,18 @@
# It is used as a unique identifier for this instance of apt::key. It gets
# hashed to ensure that the resource name doesn't end up being pages and
# pages (e.g. in the situation where key_content is specified).
$digest = sha1("${key}/${key_content}/${key_source}/${key_server}/")
$digest = sha1("${$upkey}/${key_content}/${key_source}/${key_server}/")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it shouldn't do the right thing. ${$upkey} should be ${upkey}, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, forced pushed

@haus
Copy link

haus commented Mar 15, 2012

So would changing all of the greps to grep -i not work in this case?

@blkperl
Copy link
Author

blkperl commented Mar 15, 2012

marut thought it would better to use stdlib's upcase method rather than adding more flags to grep.

Maybe @marut can explain his thought process. I've already forgotten the reason :)

Previously lowercase keys would be installed every
puppet run because apt-key list returns an uppercase
key. This commit makes the comparison case insensitive.
haus added a commit that referenced this pull request Mar 15, 2012
(#13125) Apt keys should be case insensitive
@haus haus merged commit a11a7b0 into puppetlabs:master Mar 15, 2012
@reidmv
Copy link

reidmv commented Mar 15, 2012

I don't have any strong feelings on the grep -i option. I usually operate with the starting perspective that I trust puppet to be consistent more than I trust exec'd commands to be consistent, hence the suggestion to explore upcase'ing instead of grep -iing. It's all largely moot now but in hindsight the upcase solution is sufficiently more complicated than the grep -i option would have been that K.I.S.S. sez grep -i would have been better; certainly from a maintainability standpoint. @blkperl, if you want to push a revert,grep -i pull request I'd support it. Not a priority though, the current solution is functional, just complicated. Sorry for pushing you in that direction.

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