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

Rename 'install' command as 'init' #20

Merged
merged 1 commit into from
Jun 4, 2019
Merged

Rename 'install' command as 'init' #20

merged 1 commit into from
Jun 4, 2019

Conversation

dpordomingo
Copy link
Contributor

fix #11

After #2, the install subcommand is not only used for the first time you run source{d}, but also to come back to a dir that was used previously.

As described in #11 I choose init, but I'm opened to suggestions.

@dpordomingo dpordomingo added the enhancement New feature or request label Jun 3, 2019
@dpordomingo dpordomingo self-assigned this Jun 3, 2019
Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@smacker smacker left a comment

Choose a reason for hiding this comment

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

After more thinking a bit more about it I favor up instead of init. I also think it should be recommended way to start Source{d} instead of init and then start.

The main difference between up and start in docker-compose that the first one recreates containers if they were updated. So if an user downloads new docker-compose.yml or makes manual changes up must be run, not start or changes in yml won't have any effect.

The name init sends different message as you have to run it only once for each dataset.

@carlosms
Copy link
Contributor

carlosms commented Jun 4, 2019

To me both up and init make sense, and to me they both give a similar message. I'm ok with any.

The main advantage I see for up is that for someone familiar with docker-compose the meaning and implications are automatically clear without having to go through our docs.

So from my side 👍 if @dpordomingo doesn't mind to change the PR.

About

I also think it should be recommended way to start Source{d} instead of init and then start.

I thought this was already the case, current install is the only thing you need. If the docs give the idea that you need to do install + start we should make it clear that start is only useful after a stop.

Copy link
Contributor

@smacker smacker left a comment

Choose a reason for hiding this comment

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

Hm. On another hand for install (or init or up) user have to pass dir to repositories which is annoying. Which means better let user execute this command only once. So then I'm indifferent to init or up.

But we definitely need to think a bit more about how to re-create containers.
It is easy with only update command. But we also have sourced compose download and sourced compose set which right now don't work as expected. (At least not as I expect)

@carlosms
Copy link
Contributor

carlosms commented Jun 4, 2019

I created a new issue so we don't loose this conversation about missing container recreate: #27

Since this command can be used to change between working dirs,
what is happening in behind is that sandbox is stopped and inited again

Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
@dpordomingo
Copy link
Contributor Author

Given the current state of the discussion, with no strong opinions about using up instead of up, and given that we have three use cases, I'd merge with init, so it could be:

  • init: to initialize the sandbox with the passed working directory,
  • to start the services with the current working directory,
    • up: after being updated (e.g. downloading a new docker-compose.yml)
    • start: after being manually stopped with stop

@dpordomingo dpordomingo merged commit 7c69195 into src-d:master Jun 4, 2019
@dpordomingo dpordomingo deleted the rename-install branch June 18, 2019 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename install to something more intuitive
3 participants