Skip to content

PE-40175 #98

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

Merged
merged 1 commit into from
Feb 24, 2025
Merged

PE-40175 #98

merged 1 commit into from
Feb 24, 2025

Conversation

taikaa
Copy link
Contributor

@taikaa taikaa commented Jan 21, 2025

Prior to this commit the sshkey type didn't prohibit whitespace in the key property. This change now fails the resource type if a user adds space and adds a spec test

@taikaa taikaa requested a review from a team as a code owner January 21, 2025 18:46
@mhashizume
Copy link
Contributor

Thanks for the PR @taikaa ! I think we only want to check for leading or trailing whitespace for keys. IMO the documentation for this type is misleading, particularly:

The key attribute may not contain whitespace.

And

Key identifiers / comments, such as 'joescomputer.local' --- put these in the name attribute/resource title."

Seem like they were copy/pasted from ssh_authorized_keys. However, the file formats for /etc/ssh/ssh_known_hosts and ~/.ssh/authorized_keys are different.

We cannot put comments in the name attribute/resource title for sshkey the way we can with ssh_authorized_keys because the hostname needs to go there. As such, we probably don't want to be as strict about the contents of the keys itself and allow for comments.

@taikaa
Copy link
Contributor Author

taikaa commented Feb 19, 2025

Hi @mhashizume thanks for your review! I modifying the test and failure condition. I also updated REFERENCE.md - once this has been reviewed I can squash my commits into one

@mhashizume mhashizume added the bug Something isn't working label Feb 19, 2025
@mhashizume
Copy link
Contributor

I have a minor request about the REFERENCE.md but otherwise things look good!

Prior to this commit the sshkey type didn't prohibit leading or trailing
whitespace for the key property. This change now fails the resource type
if a user adds space and adds a spec test.
@mhashizume
Copy link
Contributor

Thanks @taikaa !

@mhashizume mhashizume merged commit 6e4be33 into puppetlabs:main Feb 24, 2025
14 checks passed
@kenyon
Copy link
Contributor

kenyon commented Feb 24, 2025

A better PR title for the changelog would be good.

@mhashizume
Copy link
Contributor

@kenyon You're right, I'll keep that in mind for next time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants