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

[ssh] Add User .ssh Config File Option #3298

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

dtzhou2
Copy link

@dtzhou2 dtzhou2 commented Jul 3, 2023

Resolves issue SUPDEV-137.
Adds a plugin option for the ssh module, defining whether it will or will not collect .ssh config files per user.
Default for new option is True

Signed-off-by: Daniel Zhou [email protected]


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • [ X] Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • [X ] Is the subject and message clear and concise?
  • [ X] Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • [ X] Does the commit contain a Signed-off-by: First Lastname [email protected]?
  • [ X] Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-3298
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@dtzhou2 dtzhou2 force-pushed the dzhou-add-ssh-plugin-option branch 3 times, most recently from 23299ec to 8628e0a Compare July 3, 2023 17:30
@arif-ali
Copy link
Member

arif-ali commented Jul 4, 2023

is it worth adding the if statement before the function call for self.user_ssh_files_permissions() instead in the function itself?

if self.get_option('userconfs'):
    self.user_ssh_files_permissions()

The function is not collecting anything else anyhow, but still doing some work; so I think this should be a better way imho

@@ -18,6 +18,12 @@ class Ssh(Plugin, IndependentPlugin):
plugin_name = 'ssh'
profiles = ('services', 'security', 'system', 'identity')

option_list = [
PluginOpt('userconfs', default='True', val_type=str,
Copy link
Member

Choose a reason for hiding this comment

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

default should be a boolean here, not a string. We do string conversion to bools from the CLI, but internally we should still be using the correct native type.

Copy link
Author

Choose a reason for hiding this comment

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

fixed issue!

@dtzhou2 dtzhou2 force-pushed the dzhou-add-ssh-plugin-option branch from af10609 to e033d8b Compare July 5, 2023 15:53
@dtzhou2
Copy link
Author

dtzhou2 commented Jul 5, 2023

is it worth adding the if statement before the function call for self.user_ssh_files_permissions() instead in the function itself?

if self.get_option('userconfs'):
    self.user_ssh_files_permissions()

The function is not collecting anything else anyhow, but still doing some work; so I think this should be a better way imho

yea I agree with that change

@TurboTurtle
Copy link
Member

Ack to the change, but it looks like your commit message got a bit mangled in any fixup work. Let's set that back to what you had with the original [ssh] Add User .ssh Config File Option content (you can do this with git commit --amend)

sos/report/plugins/ssh.py Fixed Show fixed Hide fixed
@dtzhou2 dtzhou2 force-pushed the dzhou-add-ssh-plugin-option branch 4 times, most recently from 9e674c1 to ae105e5 Compare July 5, 2023 19:42
@TurboTurtle
Copy link
Member

Looks like the commit message got murky again in a follow up. To be clear - the commit title should be what is now on the first line ([ssh] Add User .ssh Config File Option). I know this is nitpicky, but this conformance really does help when trawling the git history.

Resolves issue SUPDEV-137.
Adds a plugin option for the ssh module, defining whether it will or will not collect .ssh config files per user
Default for new option is True

Signed-off-by: Daniel Zhou <[email protected]>
@dtzhou2 dtzhou2 force-pushed the dzhou-add-ssh-plugin-option branch from ae105e5 to 32537bb Compare July 7, 2023 19:16
@dtzhou2
Copy link
Author

dtzhou2 commented Jul 7, 2023

Apologies for that. Don't know why my git commits keep on getting messed up titles. Just fixed

@TurboTurtle TurboTurtle merged commit 24b6507 into sosreport:main Jul 7, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants