Skip to content

Conversation

@roopakparikh
Copy link
Contributor

This change introduces the use of SSHClient and simple library
that uses golang ssh client to execute commands remotely.

I have tested prep-node remotely, but nothing else. Will be doing
more refactoring as more testing is performed.

cmd/prepNode.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I would return an error here and not Fatalf ( FatalingF at top layer ). Wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I debated that, didn't see a point in doing that, but if you feel it is inconsistent, I can change it. Here is the reason why I did it this way.

  • This is part of the file that is supposed to be doing validation and preparation.
  • doing so in other places would be unnecessary, I will probably return and error with the same string and the function above will do a fatalf.
    if this check was in another file, I would have absolutely agreed with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok @abhimanyu-babbar I have refactored it little bit PTAL

This change introduces the use of SSHClient and simple library
that uses golang ssh client to execute commands remotely.

I have tested prep-node remotely, but nothing else. Will be doing
more refactoring as more testing is performed.
Copy link
Contributor

@abhimanyubabbar abhimanyubabbar left a comment

Choose a reason for hiding this comment

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

Changes looks good to me 👍 :shipit: 🎉

@roopakparikh roopakparikh merged commit 23b72ea into master Oct 28, 2020
@roopakparikh roopakparikh deleted the private/rparikh/ssh branch October 28, 2020 18:47
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