search

phase2 / rig

Outrigger command line tool
MIT License
11 stars 8 forks source link

project sync executed in project root directory #86

Closed grayside closed 7 years ago

grayside commented 7 years ago

Corrects the assumption in project_sync.go that commands should be executed from the current working directory. Instead, commands are executed the same as rig project scripts -- from the directory in which the configuration file is found.

Fixes #67

grayside commented 7 years ago

This needs more testing for execution when there isn't any project configuration.

grayside commented 7 years ago

The change as implemented removes support for the current working directory as a fallback to their being no configuration files available, so that needs to be put back.

febbraro commented 7 years ago

So you are saying that before this PR the root for the sync dir was whatever dir you were in when you ran the command, and not the directory that the outrigger.yml file is in?

Now with this PR it will only work if there is an outrigger.yml and use that file's directory, but we also need to account for the current working directory if that project config file doesn't exist?

Also, with the issue you linked, recursive project config discovery, that seems kind of problematic. Maybe the project config file need to specify the sync dir, and if there is no project config in the recursive heirarchy that specifies a sync dir then it just uses the current dir?

grayside commented 7 years ago

So you are saying that before this PR the root for the sync dir was whatever dir you were in when you ran the command, and not the directory that the outrigger.yml file is in?

Correct, if you look at the code, it used os.Getwd() pretty ubiquitously.

Now with this PR it will only work if there is an outrigger.yml and use that file's directory, but we also need to account for the current working directory if that project config file doesn't exist?

Yes, that's what my latest comment discussed. I over-corrected. We wanted to be able to use this functionality outside a project context. That seems like an edge case (similar to us using docker run instead of docker-compose run), but still probably worth supporting for quick experiments.

Also, with the issue you linked, recursive project config discovery, that seems kind of problematic. Maybe the project config file need to specify the sync dir, and if there is no project config in the recursive heirarchy that specifies a sync dir then it just uses the current dir?

The idea is that if an outrigger.yml is found, the rig project sync should be rooted in that directory. If not, it should be the current working directory. It makes sense to me that we further enhance the outrigger.yml unison config to specify a relative path to sync from below the project config structure. Currently all rig project scripts execute in the directory of the outrigger.yml file, we assume it is always at the repo root.

febbraro commented 7 years ago

But it is possibly to have a "global" outrigger.yml, say in your $HOME dir, and it is possible without a project config (for various use cases) that it will share your $HOME dir via unison. Should we additionally add a flag that says use $CWD?

grayside commented 7 years ago

I think it makes sense to add a flag to designate the localhost side of the sync as a path. So --sync-from=. or --sync-from=$PWD would end up as the current directory, but other options are available, such as if a particular project is structured such that only the contents of a subdirectory should be synced.

In priority order, this gives us:

  1. --sync-from flag
  2. outrigger.yml
  3. Current working directory

As long as we're talking about these options, it's worth noting that we currently have an argument that causes problems for other commands to interact with the sync container, so we should be careful to keep those use cases in mind. #85 for discussion on that.

febbraro commented 7 years ago

Would --sync-path or --sync-root work better?

grayside commented 7 years ago

Re-introduced the CWD fallback, added --sync-path override, added verbose logging of what the working directory is for sync execution, followed that through to do similar in project scripts and tweaked a couple log messages for easier reading flow.

grayside commented 7 years ago
grayside commented 7 years ago
febbraro commented 7 years ago

So, it will choose either the sync-path provided, or the location of the config file, or the current working directory. Do we think that it makes sense to have a generally configurable path in the sync section of the config file? or do you think that is just generally captured under the --sync=path option and you'd encod ethat directly into your project scripts?

I will do a bit of a deeper review tonight, but i'm thinking this thing is just about good to merge, thanks for all the hard work on it.

grayside commented 7 years ago

@febbraro my thinking is that projects will always sync relative to the outrigger.yml, because in that way if you connect via the CLI container all the pieces are the same, which I find tremendously valuable for general intuition. I could see the value of a sync-path option in outrigger.yml, but given the ignorenot modifier for Unison we theoretically support it already. https://www.cis.upenn.edu/~bcpierce/unison/download/releases/stable/unison-manual.html#ignore

grayside commented 7 years ago

Travis failed the branch on a partial refactoring I had in the util directory, to take the easy way out I added those changes to this PR.

I've broken up util.go into user_input.go and docker.go and folded image.go into docker.go. When there's more than one file I think semantically naming and categorizing everything makes it easier to find.

grayside commented 7 years ago

This one wins the award for hardest easy PR I've landed in rig.