Add `kb-sdk shell` command

kbase / kb_sdk

Build and test new apps for the KBase platform

http://kbase.github.io/kb_sdk_docs

MIT License

27 stars 32 forks source link

Add `kb-sdk shell` command #314

Closed jayrbolton closed 5 years ago

jayrbolton commented 6 years ago

Here is my first shot at adding some Java. @MrCreosote let me know what feedback you have.

This opens the docker shell as a root user, which I know Shane didn't want. But it works well for me. We can update this PR later with an alternative UID. Also I think we could remove the run_bash.sh script once this update looks good.

jayrbolton commented 6 years ago

Thanks for the review. I took a lot of the code around getting the module name/directory from elsewhere (for example here). It felt pretty overly complicated and seems like it should be a lot easier. @MrCreosote maybe you have better boilerplate for passing/fetching module name/directory from the ShellLauncher class?

MrCreosote commented 6 years ago

Not off the top of my head. The first thing I'd do is isolate all that stuff in a ModuleInfo class (or something) so if there's a way to simplify it can be done in one place rather than all over.

jayrbolton commented 6 years ago

@MrCreosote I made changes and would be ready for a re-review at any point, no hurry. Change notes:

Added javadocs comments
Returning void rather than Process in ShellLauncher#exec()
Converted a lot of my variables to final (I think all of them)
Added a ModuleInfo class that gets module config and directories. Has some better error handling.
Replaced all tab characters in ModuleBuilder with four spaces

Other notes:

I will test on a mac soon
Can do a RELASE_NOTES.txt update for this and increase patch semver. Not sure what conventions you all have here.
Lots of other files mix tabs and spaces. I can fix all of them in another PR.
There is a lot of opportunity to refactor code all over the place here. Will be tricky to balance big refactors vs incremental changes on this repo

MrCreosote commented 6 years ago

I will test on a mac soon

We should do that before merge.

Can do a RELASE_NOTES.txt update for this and increase patch semver. Not sure what conventions you all have here.

Just follow the conventions in the current file, but make the release date TBD. We'll add that when we merge to master.

Lots of other files mix tabs and spaces. I can fix all of them in another PR.

Dooo eet

There is a lot of opportunity to refactor code all over the place here. Will be tricky to balance big refactors vs incremental changes on this repo

Refactor as you go IMO, otherwise you'll be refactoring for the next few months. If you're working on a chunk of code, make it better.

jayrbolton commented 6 years ago

Still todo on this one for me:

[x] test on mac
[ ] take some kind of stab at making tests for the classes created here (ModuleInfo, at least)

jayrbolton commented 6 years ago

@sychan This could potentially be merged. I've tested it quite a bit, including on mac. The remaining thing I wanted to do was take a stab at writing tests for the ModuleInfo class I added here, but didn't make a ton of headway. Trying to write tests for that class might not be worth it right now

jayrbolton commented 6 years ago

Diff without the whitespace changes to ModuleBuilder: https://github.com/kbase/kb_sdk/pull/314/files?w=1

jayrbolton commented 6 years ago

@scanon maybe you have feedback on this one

jayrbolton commented 5 years ago

:man_shrugging: