Skip to content

Read the username and port from /.ssh/config file and replace if present #659

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 28, 2021
Merged

Read the username and port from /.ssh/config file and replace if present #659

merged 7 commits into from
Apr 28, 2021

Conversation

@sanga1794
Copy link
Contributor

@sanga1794 sanga1794 commented Jan 20, 2021 •
edited by clintoncwolfe
Loading

Signed-off-by: sanga17 sausekar@msystechnologies.com

Description

Read the username and port from /.ssh/config file and replace if present

Related Issue

Fixes #101

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New content (non-breaking change)
  • Breaking change (a content change which would break existing functionality or processes)

Checklist:

  • I have read the CONTRIBUTING document.
@sanga1794
Read the username and port from /.ssh/config file and replace if present
e259c2c 
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
@sanga1794 sanga1794 requested a review from a team as a code owner January 20, 2021 12:43
@chef-expeditor
Copy link
Contributor

Hello sanga1794! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. Possible Outcomes
    a. If everything looks good, one of them will approve it, and your PR will be merged.
    b. The maintainer may request follow-on work (e.g. code fix, linting, etc). We would encourage you to address this work in 2-3 business days to keep the conversation going and to get your contribution in sooner.
    c. Cases exist where a PR is neither aligned to Chef InSpec's product roadmap, or something the team can own or maintain long-term. In these cases, the maintainer will provide justification and close out the PR.

Thank you for contributing!
@sanga1794
Rubocop offences resolved
76a9d19 
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
@sanga1794 sanga1794 changed the title Read the username and port from /.ssh/config file and replace if present WIP: Read the username and port from /.ssh/config file and replace if present Jan 20, 2021
@sanga1794 sanga1794 changed the title WIP: Read the username and port from /.ssh/config file and replace if present Read the username and port from /.ssh/config file and replace if present Jan 22, 2021
clintoncwolfe
clintoncwolfe requested changes Jan 26, 2021
View reviewed changes
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

I think this is a great contribution! However, we need to make sure that the precedence is correct. Thanks!
lib/train/transports/ssh_connection.rb Outdated
@bastion_host = @options.delete(:bastion_host)
@bastion_user = @options.delete(:bastion_user)
@bastion_port = @options.delete(:bastion_port)
check_config_options
Copy link
Contributor

Choose a reason for hiding this comment

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

I might suggest the name read_options_from_ssh_config. The word check implies that there is some validation going on.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure will do @clintoncwolfe, thanks
@sanga1794
replaced method name
aefba80 
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
@sanga1794
Copy link
Contributor Author

@clintoncwolfe please have a look on updated PR. Thanks!
@clintoncwolfe
Copy link
Contributor

This PR is waiting on an update from @sanga1794 to fix the precedence. Do you think you will have time to make the change?
@sanga1794
Copy link
Contributor Author

@clintoncwolfe I have updated the precedence, please have a look. Thanks!!
clintoncwolfe
clintoncwolfe reviewed Apr 27, 2021
View reviewed changes
lib/train/options.rb Outdated
if default.is_a? Proc
res[field] = default.call(res)
elsif hm.key?(:coerce)
field_value = hm[:coerce].call(res[:host])
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it makes sense in general to pass the hostname to the Proc. Instead, just pass the options hash (res) to the Proc, and let the Proc internally access the field that it needs (if any - here both will need host).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure i'll make those changes. Thanks @clintoncwolfe !!
clintoncwolfe
clintoncwolfe reviewed Apr 27, 2021
View reviewed changes
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Overall this looks good! I just have one minor change requested - to alter what is passed to the Proc.
@clintoncwolfe clintoncwolfe self-requested a review April 27, 2021 03:00
@sanga1794
Replaced parameter hostname with options hash
e387d6e 
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
clintoncwolfe
clintoncwolfe approved these changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

OK! Thanks for your work on this - I think it has a lot of future potential!
@clintoncwolfe clintoncwolfe merged commit dbbf39e into inspec:master Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Expeditor: Bump Minor Version Platform: SSH

2 participants

@sanga1794 @clintoncwolfe