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

Deployment reorg/refactor as groundwork for multi-language support #723

Merged
merged 19 commits into from
May 11, 2016

Conversation

rwaldron
Copy link
Contributor

@rwaldron rwaldron commented May 2, 2016

WIP

Deployment needs quite a bit of reorganization before any multi-language support can be considered. This PR will track the changes, ideally as each step in a single commit. I'm imposing a strict requirement that all changes must pass the existing tests with little or no changes to the tests (ie. no changes that will alter the operational outcome or semantics).

Much of the work entails:

  • Renaming operations to be less "script" specific.
    • However, some cases will be to make an operation that's JS-specific more clearly that.
  • Eliminating vestigial code
  • Consolidating operations wherever possible.
  • Splitting up the current code in lib/tessel/deploy.js on a "generic or not" line.
  • Splitting up the current code in test/unit/deploy.js on a "generic or not" line.

I will update this list as notable work arises

Signed-off-by: Rick Waldron waldron.rick@gmail.com

@rwaldron rwaldron force-pushed the deployment-reorg branch 2 times, most recently from 0bff2bf to 0784308 Compare May 2, 2016 16:26
};

module.exports.py = {
execute: (filepath, relpath) => ['python', filepath + relpath],
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been pruning python related code for now since nobody is working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just here as a placeholder.

@johnnyman727
Copy link
Contributor

@rwaldron this looks like a valuable endeavor.

I've was thinking that the only generic behavior that we should keep in deploy.js is transferring the tarball. We could also keep the logic that starts pushed scripts but it would need to be abstracted a bit to write one of a couple of options for the shell files that gets placed at /app/start so I'm not sure it's worth the common code path.

@rwaldron
Copy link
Contributor Author

rwaldron commented May 2, 2016

I've was thinking that the only generic behavior that we should keep in deploy.js is transferring the tarball.

Transferring, yes. Creating: no.

@rwaldron
Copy link
Contributor Author

rwaldron commented May 4, 2016

Unless we select a "default language", this is no longer possible:

t2 run . 

@johnnyman727
Copy link
Contributor

we can't detect a Cargo.toml or Package.json?

@rwaldron
Copy link
Contributor Author

rwaldron commented May 4, 2016

we can't detect a Cargo.toml or Package.json?

I guess if we special case that scenario. :(

The reason this no longer is works is because the whole process needs to already know what language it wants by the time it gets to findProject, because there are specific considerations that findProject needs to account for, that are per-language.

@rwaldron
Copy link
Contributor Author

rwaldron commented May 4, 2016

...That still doesn't work. How does t2 run . know which of these python scripts is the entry point:

$ tree
.
├── README.md
├── __init__.py
├── blinky.py
├── setup.py
└── tessel.py

So...

  • __init__.py is empty
  • setup.py is some junk I made
  • Maybe blinky.py or tessel.py? No way to know.

@rwaldron
Copy link
Contributor Author

rwaldron commented May 4, 2016

@johnnyman727 Almost there. I did not implement Rust deployment, I'll leave that to you. I did implement your changes from findProject, those are in https://github.com/rwaldron/t2-cli/blob/deployment-reorg/lib/tessel/deployment/rust.js, which gets called from findProject. I believe you want to put the contents of this: https://github.com/tessel/t2-cli/pull/511/files#diff-191f4ebe176e9e816abc0445a664be98R272 into here: https://github.com/rwaldron/t2-cli/blob/deployment-reorg/lib/tessel/deployment/rust.js#L48-L50

@rwaldron
Copy link
Contributor Author

rwaldron commented May 4, 2016

@johnnyman727 ready for review.

@johnnyman727
Copy link
Contributor

@rwaldron I won't be able to review until Sunday but I am excited about it.


// Internal
var commands = require('./commands');
var deployLists = require('./deploy-lists');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to specific language deployment module

var logs = require('../logs');
var Preferences = require('../preferences');
// Necessary to ensure that the next line has had the LOCAL_AUTH_PATH descriptor added.
var provision = require('./provision'); // jshint ignore:line
var Tessel = require('./tessel');

var binaryModulesUsed = new Map();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to lib/tessel/deployment/javascript.js

@johnnyman727
Copy link
Contributor

@rwaldron sorry, finally got time to take a look!

It looks great with a few minor issues as commented. I was able to reproduce your smoke tests and the path to adding Rust support is clear and logical. 👍

…mpleExec

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
…ecific deployment commands

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
…TestCode

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
…ipt" centric naming

Internals

- Renames setExecutablePermissions => chmod; Change signature: (filepath) => (permssion, filepath)

Bindings (misc)
- t => tessel

APIs

- Tessel.prototype.deployScript => Tessel.prototype.deploy
- Tessel.prototype.restartScript => Tessel.prototype.restart
- controller.deployScript => controller.deploy
- controller.restartScript => controller.restart
- (deploy|actions).runScript => (deploy|actions).run
- (deploy|actions).pushScript => (deploy|actions).push

Additionally:

- Removes unused `script` argument at actions.run/actions.push callsites and defintion

Tests:

- Primarly updating names
- Two cases update the expected arguments for the removal of the unused `script` argument

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
…-specific operations and data

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
…avascript.js

Still more granular work to do, but this gets the split in.

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
- deployment.*.lists tests
- move js specific stuff: test/unit/deploy => test/unit/deployment/javascript

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
@rwaldron
Copy link
Contributor Author

@johnnyman727 all review notes are addressed and this has been rebased

@@ -24,6 +24,7 @@ exports['deployment.resolveLanguage()'] = {

jsInferred: function(test) {
test.expect(4);
// Set the stub return value
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the helpful comments

@johnnyman727
Copy link
Contributor

johnnyman727 commented May 11, 2016

Looks good! Let's merge this behemoth. (assuming Travis gives the green)

@rwaldron rwaldron merged commit 24fdd7d into tessel:master May 11, 2016
@johnnyman727
Copy link
Contributor

👏

@rwaldron
Copy link
Contributor Author

🎉

1 similar comment
@wyze
Copy link
Member

wyze commented May 11, 2016

🎉

@tikurahul
Copy link
Contributor

@rwaldron Please take a bow. This is totally awesome.

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