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

(MODULES-10867) Ensure ssh key name is unique based on type, content and description #340

Merged
merged 6 commits into from
Dec 18, 2020

Conversation

mdklapwijk
Copy link
Contributor

Allow multiple ssh keys of the same type having the same description.

@mdklapwijk mdklapwijk requested a review from a team as a code owner November 12, 2020 09:50
@puppet-community-rangefinder
Copy link

accounts::manage_keys is a type

that may have no external impact to Forge modules.

This module is declared in 3 of 575 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@pmcmaw
Copy link
Contributor

pmcmaw commented Nov 16, 2020

Hey @mdklapwijk

I would like to thank you for taking the time to both file a ticket and also create a PR for this issue.

Would it be possible to address the failing test: manifests/manage_keys.pp - WARNING: variable not enclosed in {} on line 42.

It would also be great if you were able to

  • Add a test to avoid any regressions in the future for this change
  • Add some documentation on your change

Many thanks
Paula

@mdklapwijk
Copy link
Contributor Author

Paula, thanks for the suggestion, missed that completely.

Use '_' instead of '-' to conform to the used naming convention.
@codecov-io
Copy link

codecov-io commented Nov 18, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@f23158f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##             main    #340   +/-   ##
======================================
  Coverage        ?   0.00%           
======================================
  Files           ?       2           
  Lines           ?      30           
  Branches        ?       0           
======================================
  Hits            ?       0           
  Misses          ?      30           
  Partials        ?       0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f23158f...980df5a. Read the comment docs.

@david22swan
Copy link
Member

@mdklapwijk
Taking a quick look at this, would just like to reiterate what has already been said.
The change look's good overall but some test coverage and documentation would be needed in order to get it in.

@mdklapwijk
Copy link
Contributor Author

mdklapwijk commented Nov 24, 2020

@david22swan, I am not sure how to solve the "Missing base report; Unable to compare commits because the base of the pull request did not upload a coverage report." message/error from codecov. Looking at your own pull request you do not seem to know a solution to this either:
puppetlabs/puppetlabs-tagmail#182

It seems the only way to solve that is to close this PR and create a new one? Please advise.

And what do you mean about documentation? It is a bug-fix not a new feature, so all info is contained within the ticket:
https://tickets.puppetlabs.com/browse/MODULES-10867

@sanfrancrisko sanfrancrisko self-assigned this Nov 30, 2020
This commit adds tests to verify the previous commit's change. We
should expect the md5 hash to be part of the ssh_authorized_key
title.
@sanfrancrisko
Copy link
Contributor

Hi @mdklapwijk ,

Don't worry about the codecov error for the meantime - we'll seek to rectify that at a future date. It appears as though some of our modules do not have a base commit from which codecov can compare coverage to. This will likely need some discussion internally on the team as to what is the best point to create that from.

Thanks for the fix - as mentioned, it looks good, however, it would be nice if we could get a test around this new functionality. I've expanded one of the existing tests which should cover us, but feel free to split out or expand further: mdklapwijk-forks#1

…_uniq_ssh_key

(MODULES-10867) Add tests to verify ssh_authorized_key unique title
@mdklapwijk
Copy link
Contributor Author

@sanfrancrisko, thanks. That made it a lot more clear what was expected!

Closing curlies needed some space...
@sanfrancrisko sanfrancrisko changed the title Ensure ssh key name is unique based on type, content and description [MODULES-10867] (MODULES-10867) Ensure ssh key name is unique based on type, content and description Dec 18, 2020
@sanfrancrisko sanfrancrisko merged commit a12c3f0 into puppetlabs:main Dec 18, 2020
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.

5 participants