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

T2 root #290

Closed
wants to merge 43 commits into from
Closed

T2 root #290

wants to merge 43 commits into from

Conversation

Student007
Copy link
Member

Purpose

This pull request will add the t2 root command to the t2-cli.

Side effects

  • test/unit/deploy.js: console.log('testing deploy') // due to frequently linter bleating
  • same in test/common/tessel-classic.js
  • changes of test/unit/provision.js are caused by automatic linter activities ...
  • test/unit/usb_connection.js is changed due to changes in lib/usb_connection.js (err -> warn)

New Files

  • lib/root_controller.js

Everything else is located in bin/controller.js

New Test files

  • test/unit/menu.js -> unit tests for Menu creation
  • test/unit/seekTessels.js -> unit tests for seeking Tessels with root command
  • test/unit/root.js -> root ssh without menu (single Tessel found)

Note: Tests run successfully on all reference machines (Mac OS X Yosemite, Windows 7 Professional, Ubuntu 14.04)

Fixme

There is no handler for managing a ssh-connected Tessel is powering off ! Needs to be fixed within the firmware if necessary... (Terminal freezes imediatelly after poweroff the (virtual) Tessel)

Improvements

The style of the menu needs a little bit love. It is reduced to functional working mode due to expectation it maybe would be replaced by a more complex and useful terminal-menu what is also providing realtime data of Tessels are found.

Usabiliy

Due to some mistakes I did during my journey/while learning curve .... there are some feedbacks/tips and a debug mode:

  • netmask is the limit
  • network topology could cause a single Tessel is found twice (this should be added to the list cmd also)

TODO

Because I do not own at least one Tessel 2 ... I am not able to check how it works with multiple physical Tessels 😿

like described here:
http://docs.travis-ci.com/user/migrating-from-legacy/?utm_source=legacy-
notice&utm_medium=banner&utm_campaign=legacy-upgrade
but without tests and only tested on MacOSX.
JavaScript Clean Styling … is a good thing. But for getting an
individual cli help we need newlines \n … what isn’t allowed by the
JSCS …
Attention: Please notice comments within tests ! There are many TODOs…
@Student007 Student007 mentioned this pull request Sep 9, 2015
var default_id_rsa = '~/.tessel/id_rsa';
var functional_msg = '\nGain SSH root access to one of your authorized tessels (menu listing if multiple targets)';
parser.command('root')
.usage(functional_msg + '\n\nUsage: t2 root [-i PATH] [--help]\n\n-i PATH: Optional targeting a different Private Key \n\n(Note: default target created by "t2 key generate" is ' + default_id_rsa + ')\n')
Copy link
Contributor

Choose a reason for hiding this comment

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

@johnnyman727 eventually we're going to need to put these icky strings into template files and load them in at runtime

Copy link
Contributor

Choose a reason for hiding this comment

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

@Student007 there is no need to add the usage function here. nomnom takes care of it for you as long as you add help tags to all the parameters. Check out the other commands for an example.

@rwaldron ugh agreed!

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is, it would write Usage: node t2 root instead of t2 root - I like the idea of templates.
@Frijol it looks like you are figuring out some things at #274 - is there also a general way to fix the node t2 root problem ?

Copy link
Member

Choose a reason for hiding this comment

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

If I have:

parser.command('root')
  .option('path', {
    abbr: 'i',
    full: 'path',
    metavar: 'PATH',
    default: '~/.tessel/id_rsa',
    help: 'Private Key (Note: created by "t2 key generate")'
  })
  .callback(function(opts) {
    controller.root(opts)
      .then(closeSuccessfulCommand, closeFailedCommand);
  })
  .help('Gain SSH root access to one of your authorized tessels (menu listing if multiple targets)');

I get:

➜  t2-cli git:(t2-root) ✗ t2 root -h

Usage: /usr/local/bin/iojs t2 root [options]

Options:
   -i PATH, --path PATH   Private Key (Note: created by "t2 key generate")  [~/.tessel/id_rsa]

Gain SSH root access to one of your authorized tessels (menu listing if multiple targets)

What's the issue? You want to get rid of /usr/local/bin/iojs part?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Frijol yes 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

what would be a new issue / contribution task

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Yeah, that should be a separate issue. Good idea. A brief look through nomnom.js doesn't reveal a solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

(I couldn't find a provided solution in nomnom.js ... also reading code ... did this before some weeks)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Student007 what do you think about leaving the usage string our for this PR and then we'll make a separate PR that grabs all strings from a template file. I'd rather keep them all consistent at any one time so that it's more straightforward for new contributors.

Copy link
Member Author

Choose a reason for hiding this comment

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

since 6 days Nomnom is deprecated !

I've figured out ... and created a pull request:
#310
The reason I though it doesn't work was to use the "script" part on on the wrong object level.

Student007 added a commit to Student007/t2-cli that referenced this pull request Sep 14, 2015
tessel#290 (comment)
- haves as discussed:
https://github.com/tessel/t2-cli/pull/290/files#r39119278
- home / ~ conversion as discussed:
https://github.com/tessel/t2-cli/pull/290/files#r39067831
- controller.closeTesselConnections changed Tessel object to use
lanConnection instead connectionType === Lan
- FIXME: The check whether a connection is authorized or not has to be
handled different due to security. USB connecting easily makes it
possible to hack IoT devices what isn’t acceptable.
- FIXME: If I use -i ~/.tessel/id_rsa_backup (a copy of erased id_rsa I
the t2 root menu shows „not authorized“ Tessels … but I can select and
connect !!! )
@johnnyman727
Copy link
Contributor

@Student007 should we close this PR in favor of #307?

@johnnyman727
Copy link
Contributor

Closing in favor of reviewing #307 and various other PRs this was broken into.

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.

None yet

4 participants