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

Add "webgme dockerize" command to generate #252

Merged
merged 13 commits into from Sep 19, 2018
Merged

Conversation

pmeijer
Copy link

@pmeijer pmeijer commented Sep 14, 2018

> webgme dockerize --help

  Usage: webgme-dockerize [options]

  Options:

    -h, --help          output usage information
    -p, --production    If set will include templates for a proxy and authentication
    -f, --force-update  If set will overwrite any existing files

This PR also fixes #251 and #250

}


DockerizeManager.prototype._go_to_root_dir = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you inherit from the ComponentManager (like here) then you can just use the _preprocess fn to wrap it and won't need to rewrite the preprocessing fn-ality. This basically ensures that functions that need to run within an existing webgme app do.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't think this command fit within Components so I decided to keep the implementation separate.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense but I am still torn bc I don't like having two implementations for the same thing...

What if you just "inherited" that method? Something like:

DockerizeManager.prototype._preprocess = ComponentManager.prototype._preprocess;

In the future, we could more _preprocess to be a utility rather than a method defined by the Component base class.

Copy link
Author

Choose a reason for hiding this comment

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

I did look into that option. However _preprocess does more than just navigate to the root-dir. It process webgme-setup.json.
I could, however, extract a util function changeDirToRoot (_go_to_root_dir no idea why I made this into snake_case btw).

@brollb
Copy link
Contributor

brollb commented Sep 16, 2018

Would you mind moving the fix for #250 and #251 to their own PR's? It will make it easier to squash them into one when I accept it. If not, it's not that big of a deal - I just like to have a PR address only a single issue to make it easier to review and so I can squash all the commits into a single commit fixing the given issue (github's PR will still preserve the individual commits despite the squash).

Otherwise, it looks pretty good! I might give it one more look over and play with it some myself but the code looks pretty good!

@pmeijer
Copy link
Author

pmeijer commented Sep 17, 2018

Is it fine if I do that next time? They were things that came up while I was working on the dockerize command and to avoid the overhead of switching branches I just put them in the same branch. You are right though - that it's better to keep them separate.
Btw Brian, do you still want PRs from us? Or do you prefer us managing the cli from now on?
Appreciate you taking the time to check this one out!

@brollb
Copy link
Contributor

brollb commented Sep 17, 2018

Yeah, no problem.

I am happy to still review PRs and stuff. However, if you need something faster (and I haven't reviewed it quick enough) don't feel like you have to wait for me. In other words, I would be happy to continue to review PRs but I don't want to get in the way of any urgent fixes/slow development (esp. since my time is a little more divided now).

@pmeijer
Copy link
Author

pmeijer commented Sep 17, 2018

Extracted changeToRootDir and fixed failing test.

@brollb brollb merged commit 7091ded into master Sep 19, 2018
@brollb brollb deleted the add-dockerize-command branch September 19, 2018 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"webgme init" should also add webgme as a devDep
2 participants