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 rig project run #29

Merged
merged 50 commits into from
May 22, 2017
Merged

Add rig project run #29

merged 50 commits into from
May 22, 2017

Conversation

grayside
Copy link
Contributor

@grayside grayside commented May 7, 2017

Thanks to febbraro for getting the branch started. I am opening this for initial feedback but still expect to work on some refinements.

This PR adds a new rig project command which wraps project-configuration of scripts as subcommands of the CLI.

  1. The configuration file has a schema version like docker-compose.
  2. There is a "scripts" top-level element to contain scripts in an npm-like manner.
  3. You will note a documentation one-liner about "namespace", that's not used yet and might go away before I'm done, it's a thought I noted down but I think generator stuff might be better.
  4. You can designate the config file location via an environment variable.
  5. Scripts are executed in the working directory of the config file.
  6. Scripts should be windows compatible, and for *nix environments at least support cshell business with concatenating commands.
  7. In the main rig help, I created a category for "Development" under which project and watch are grouped. I debated adding doctor, or creating other categories, but left that to other tasks.

TODOs

  • Add support for multi-line scripts. (Convert loop to concatenation)
  • Fix config file-based crash when file does not exist. (Separate PR)
  • Pass additional arguments through to the project script to like aliases "rig project r npm install"
  • Be able to open a standing "session" in a docker container. (Build container cli)
  • go fmt via the docker stuff is being all no-op, so I'll need to get it fixed or set up golang locally.
  • Finish util.PassthruCommand() and it's usage to pass the exit code from the script through
  • Add an example project config file.
  • Fix muddled YAML & project config processing
  • Decide if command configuration should support alias, description, multiple OSs, multiple lines
  • Decide if "rig project" should itself be the "rig project run" command, and push the other project-scoped command concepts to the top-level.
  • Add support for alias & description script config.
  • Switch to rig project, removing the run subcommand.
  • Configure a bin-dir in the outrigger.yml and add that bin dir to the PATH.

Follow-ups

Once this is merged we'll be in a position to file tickets. Filing tickets is sufficient to check off the boxes.

  • Directory traversal to find the nearest config file. (Can this just be handled in wrapper bash functions?)
  • Add documentation to outrigger-docs.
  • Add .outrigger.yml to appropriate outrigger-example instances
  • Create a new command rig aliases that can be sourced... eval $("rig project aliases")
  • Add support for config file overrides (for local overrides explicitly or multiple?)
  • Add an OS override YAML

Follow-up Aliases approach

(This probably belongs somewhere else, but jotting this down while I have a text area for now.)

A rig aliases system could be as simple as taking all the scripts, and proposing aliases such as:

alias r<key>=<value>

So in that case, suppose we had this:

version: 1.0
scripts:
  r: docker-compose -f build.yml run --rm
  d: docker-compose -f build.yml run --rm drush
  logs: docker-compose logs
  clean: docker ps -a --filter="label=sh.outrigger.vm=<project>" | xargs docker rm

We would then get these for aliases with this proposal:

alias rr="docker-compose -f build.yml run --rm"
alias rd="docker-compose -f build.yml run --rm drush"
alias rlogs="docker-compose logs -ft --tail=10"
alias rclean="docker ps -a --filter=\"label=sh.outrigger.vm=<project>\" | xargs docker rm"

This has the virtue of keeping the rig scripts and aliases lined up and letting the project control most of it. If we have any notions for "build-in" aliases, we could have them but let the scripts configuration override the command.

@tekante
Copy link
Member

tekante commented May 8, 2017

Regarding point 4, being able to specify the config file name seems like a nice to have though if it's working great. It's not clear to me from the code (as I'm not as go proficient as I'd like) whether directory ascension is being done to search for a config file though given the function of the variable I suspect it isn't. I think that would be more useful as then one could run things like rig project run r from anywhere in the source tree and it would just work (assuming the config file was in an ancestral directory.

Regarding point 6, agreed in concept on OS compatibility though I think that can be left to a project level decision though we document the recommendation. I think it's fine to require posix compatible tools though even in cross os scenarios for a project. Most specifically, the find command on windows is abysmal.

Regarding aliases, would we not recommend global aliases that make use of expected rig aliases? I can see why alignment but using the raw command is useful for projects that don't define aliases. If we build in a set of default ones the global aliases should use them. Clean and logs seem to be the only ones that don't assume Phase2 practices though. If combined with directory ascension looking for the config file that makes an alias like rr (expanding to rig project run r which then expands to find the config file located at the root with the build.yml file) more useful.

@grayside
Copy link
Contributor Author

grayside commented May 8, 2017

It is not performing directory traversal. We could consider doing so in a followup, but I felt like a first step was just a bash helper function to do something like find the .git directory, check if a .outrigger.yml is next to it, and then invoke rig with that path. We can take it all the way as part of rig, but that seems like a follow-up.

The numbered list contains things that seem to be working for me in my local OSX testing.

@tekante
Copy link
Member

tekante commented May 8, 2017

Agreed on the follow up status for directory searching for config file, I wouldn't block this on that.

Looking forward we may want to consider whether the environmental variable only contain the name of the file to use. Not conflating path and name may be useful when directory traversal is implemented. Though I can see benefits to having an option to have a path prefix so that the config could be stored relative to a directory that would be traversed. For example REPO_ROOT/outrigger/outrigger.aliases.yml in an Acquia repo with an environmental setting of outrigger/outrigger.aliases.yml such that when checking after walking up to the root of the repo it gets found. I think I've basically talked myself out of recommending any change to what can get put in the environmental variable as I've typed out possibilities here.

Copy link
Member

@febbraro febbraro left a comment

Choose a reason for hiding this comment

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

Left some feedback and questions.

cli/util/yaml.go Outdated
"gopkg.in/yaml.v2"
)

type ProjectConfig struct {
Copy link
Member

Choose a reason for hiding this comment

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

Why would these not be left in Project? or are you imagining this growing bigger than just project config in the future? If so, we may want to create a config util as opposed to yaml (or both)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking this would head to both. I can imagine having a rig-wide config that allows controlling default behaviors on both project config and non-project config.

This is ProjectConfig here because the struct started as P, and my refactoring only took the intermediate step. This should probably just be interface in this file, and in a new project-config.go or in the existing project.go remap it to the more explicit data structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a lot of trouble figuring out how to parse YAML without defining it's struct as part of the initial object. It ended up feeling cleaner & easier to just pour all the YAML handling into the new project-config.go

return err
}

func PassthruCommand(cmd *exec.Cmd) (stdout string, stderr string, exitCode int) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a function commend for this one so we know the intent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I can. This function is not yet working as intended so I'll be back to it :)
It started (pretty obviously) with http://stackoverflow.com/a/40770011/38408

},
}

command := cli.Command{
Copy link
Member

Choose a reason for hiding this comment

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

So what happens if you just call rig project? I was thinking this would list out all the scripts to run.

rig project list all scripts
rig project build would run the build script
rig project clean would run the clean script

You are anticipating running rig project run to get a list of all scripts? and rig project run build to run the configured build script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rig project run lists the scripts. rig project run build would run the build script.

I wanted to have rig project create to run the generator, and possibly other "built-in" commands that are project centric.

There could also be rig project aliases and so on. If we are okay with rig -h getting longer at the top, we could expand on the "Development" help category and just have create-project, project-aliases, etc as top-level.

I was also making space if we ever wanted to implement lifecycle scripts (maybe via a hook or events key) to do things like pre-start, post-start around rig start. But again, we can implement any commands that might generate on the top level.

@mike-potter
Copy link
Member

  1. I assume scripts can have more than one line? Might show an example of that. (more of a doc issue)

  2. I think I would like to see more structure within the "scripts". If we auto-generated aliases I would like to specify the alias in the config file. I don't like to see commands like "rig project r" instead of "rig project run" but might still want "rr" to be the alias.

  3. Related to 2 and regarding the OS compatibility, I agree it should be up to the project, but I wonder if we can help support this by looking for an OS-specific version of a command. We could use the OS as a key for an OS-specific version:

scripts:
  doit:
    alias: it
    default:
      - cd somewhere
      - cp source dest
      - ls source
    win:
      - cd somewhere
      - copy source dest
      - dir source

This also allows us to add additional meta data in the future such as "help" which could be used in a "rig project --help" output to describe each command, etc. Just having the script as the only value for the key just limits future applications I think.

@grayside
Copy link
Contributor Author

grayside commented May 8, 2017

Background thought: One of the subtexts of the conversation is that YAML parsing in a strongly typed language like go is a bit more complex than in scripting languages, so accepting variations or more complex structures takes more effort.

I assume scripts can have more than one line? Might show an example of that. (more of a doc issue)

Nope, it currently expects a single string. For more complexity in the command I think it should be committed as a bash script in a bin/ directory anyway. Is there some value-add rig can provide by accepting an array there?

Also, there are no docs yet. The example in the code is mostly a note from Frank to me than even a finalized code comment :)

I think I would like to see more structure within the "scripts". If we auto-generated aliases I would like to specify the alias in the config file. I don't like to see commands like "rig project r" instead of "rig project run" but might still want "rr" to be the alias.

I'm not sure I'm clear on why something has value as an alias but not as a script. I figured just make everything a script and make all those things a simpler alias as well. Then have a generator prepopulate the more essential scripts/aliases we want to spread around. What do you see as the driving distinction? Just having the rig project run script having a longer name for clarity than the length of a good alias?

OS Compatibility

I had the notion of enabling an .outrigger.local.yml to merge on top of the main file. Would it make sense to have an OS-specific overlay approach? 99% of the time the Windows stuff will not be used, and then the users that do will not care about the POSIX bits.

@grayside
Copy link
Contributor Author

grayside commented May 8, 2017

In response to @tekante, I was thinking we don't necessarily need to build the directory traversal into rig, as we kind of want it for rig and for docker-compose, which means maybe we just want rig aliases to be more than aliases? For example:

#!/usr/bin/env bash

rig_project_activate () {
  export GIT_REPO=$(git rev-parse --show-toplevel 2> /dev/null)
  export RIG_PROJECT_CONFIG_FILE=$GIT_REPO/.outrigger.yml
  export COMPOSE_FILE=$GIT_REPO/build.yml
}

r () {
  if [ -z "$GIT_REPO" ]; then
    rig_project_activate
  fi

  docker-compose run --rm "$@"
}

Obviously this example is super naive on the idea that you would only ever navigate to one project repo in a terminal session, but this is the kind of thing I was thinking about.

@mike-potter
Copy link
Member

mike-potter commented May 9, 2017

Sorry I wasn't clear. The purpose of the "alias" key is to specify the name of the alias you want to use instead of the default. This was in response to the auto-alias approach using alias r<key>=<value>. In my example, the auto-alias would be "rdoit" but the alias key would override that default and create an alias called "it" instead. But the script would still be available as "rig project doit".

Requiring the multi-line script to be moved to a bash file is fine but you still need the OS-specific. Because on Windows the script file would be "doit.bat" vs the "doit.sh" on linux/osx.

Supporting multiline scripts just makes this more consistent with how scripts are managed in BLT and also decreases the number of files that you need to keep track of. The command1 && command2 && command3 method for chaining multiple commands into a single line also wouldn't work on Windows, and I can forsee common cases where you need to do something simple but need to set your directory first via cd. For example, on my last Acquia project using BLT, we had several 2-3 line scripts in the project.yml configuration file.

I don't know how YAML is parsed in "go" but seems like it should not be hard to detect if the script returned an array or a string value and then deal with it.

I don't think just merging a "local" file solves the problem. If I'm a tech-lead on a project creating the outrigger config file for it, I would be setting up the scripts for all the devs to use. If we had a Windows dev on the project we would create Windows-specific versions of each script. Some developers would be on Mac and some devs would be on Windows, so "local" is different for each and could not just be committed as a single file to the code repo for the project.

@grayside
Copy link
Contributor Author

grayside commented May 9, 2017

Supporting multiline scripts just makes this more consistent with how scripts are managed in BLT and also decreases the number of files that you need to keep track of.

I'm not worried about consistency with BLT as much as npm or composer, but I do appreciate the argument that it has value.

I don't think just merging a "local" file solves the problem.

What if we supported ".outrigger.windows.yml" as something rig would override with. Is a separation like that worth it?

@mike-potter
Copy link
Member

mike-potter commented May 9, 2017

What if we supported ".outrigger.windows.yml" as something rig would override with. Is a separation like that worth it?

That's probably fine if merging is easier than just looking for the key in the main file when the script is executed, and maybe it has value if there are things other than the scripts that are OS-specific to override.

I still believe that supporting sub-keys for the scripts (like the "alias" key, "description", etc) is still a valuable architecture as it allows easier expansion and addition of script metadata in the future. If we just assume the only value of the script key is the script itself and then later decide we need metadata it would make compatibility and migrating more difficult.

@grayside
Copy link
Contributor Author

grayside commented May 9, 2017

I'm convinced about the metadata at least :) Just missing the description is one of the things I've regretted about the gdt scripts implementation.

@mike-potter
Copy link
Member

I'm not worried about consistency with BLT as much as npm or composer

My comment about BLT wasn't to imply "we have to do it this way", but just indicating that BLT in general is easier to use so we should learn from it when it makes sense, and we do potentially have projects that move between Outrigger and BLT so having similar concepts and functionality can be useful.

Also, composer does allow multiline scripts:

        "post-install-cmd": [
            "MyVendor\\MyClass::warmCache",
            "phpunit -c app/"
        ],

example from https://getcomposer.org/doc/articles/scripts.md.

I guess I'm also looking for a more compelling reason not to allow multi-line scripts other than "it is harder."

@grayside
Copy link
Contributor Author

We're simultaneously discussing what is a blocker on this PR and what the ultimate goal should be. I do still contend that if it's more commands than can gracefully be concatenated, a separate script might be valuable.

Still, here's what I propose: I am planning on this config file being versioned, which means we have a lot of flexibility to also support multiline in a followup, or mandate a multi-line style structure in a bc break. On that basis, can this be a future enhancement?

I am working on alias & description support as an extension of refactoring the YAML parsing.

@grayside
Copy link
Contributor Author

I got multiple scripts working. Will have a nice big commit once I finish up a few details and figure out how to format the help better.

@grayside
Copy link
Contributor Author

Alright, lots of changes in the last couple of commits. But also some new todos.

  1. We now have multiline (a.k.a., multi-step) scripts. However, they do not preserve any continuity from one step to the next. So you cannot cd src/themes/mytheme on one step, then run npm install on the next.
  2. We have support for aliases and descriptions, and since I had that, I decided there was more to gain from registering the configured scripts as official subcommands than trying to use the awkward-for-this-purpose, official flag system, so now the config flag is more "ad hoc".
  3. There is an example YAML file to illustrate what this looks like.
  4. You cannot run docker-compose -f build.yml run --rm cli and get a session in the build container, because the current exec model here stops all processing until execution completes. So there needs to be some work to make it not wait. My first effort on doing that with exec.Start() was a runtime crash.

@mike-potter
Copy link
Member

I think this is looking pretty good.

  1. If multiple commands cannot be run "together" (like putting ";" between each line), then we probably should add a "dir" argument to the script that lets you set the working directory for the script.

  2. Maybe add an example to the example file showing that doing "cd something" followed by a command doesn't work as people might expect.

@grayside
Copy link
Contributor Author

@mike-potter so you are suggesting we might do something like join($command_steps, '; ')?

That's an interesting idea. There's a couple downsides:

  • I'm really hoping not to hack the executed code because then this system could be a source of obscure errors.
  • I have no idea whether that's Windows compatible.

I like the idea of a dir argument to override the default behavior. That seems like it would address the main use case of having "execution continuity".

@mike-potter
Copy link
Member

In Windows you would join with ' & '. So you could set an OS delimiter the same place you set the command name.

I agree that there might be cases where this interferes, but if it's a complex set of commands that is best put in a script file anyway. Trying to accomodate the 80% use-case here. Doing a "cd" between steps is common (looking at various build scripts I've used over time).

I'm just really worried about people using "cd dir" and expecting it to work. The dir key handles an initial cd but not any other cds needed in the middle. But I could accept that solution if the join doesn't work (seems like it should though).

Copy link
Member

@tekante tekante left a comment

Choose a reason for hiding this comment

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

Had a few notes but nothing I think needs to block merging. I can submit a few of these as PRs once this one merges.

Example outrigger file needs double backslash in the Regex example line

rig project run just errors rather than printing help. I think because run is an alias of project which I assume is because that's how things need to work to get the various help pieces to print out but wasn't what I was expecting. So just to confirm, is run really supposed to be an alias for project?

Should ./bin be prepended to the path rather than appended? I would think we want things in the project to take priority on the small chance that script calls match commands that happen to already exist in a person's path.

The scripts help needs a space in the description.

@grayside
Copy link
Contributor Author

rig project run just errors rather than printing help. I think because run is an alias of project which I assume is because that's how things need to work to get the various help pieces to print out but wasn't what I was expecting. So just to confirm, is run really supposed to be an alias for project?

rig project run printing help wasn't the goal, as far as I understood. Currently rig run is an alias of rig project, mostly for a lark. The help for project scripts is all in the main rig project or rig project help response. As a followup, we could see if we can implement a specialized rig project run that provides help for the configured scripts, but for now I've been using that as a configured script itself.

Should ./bin be prepended to the path rather than appended? I would think we want things in the project to take priority on the small chance that script calls match commands that happen to already exist in a person's path.

Good call.

@febbraro febbraro changed the title Add rig project run [WIP] Add rig project run May 19, 2017
@febbraro febbraro merged commit 9b2ed15 into develop May 22, 2017
@febbraro febbraro deleted the feature/rig-project branch May 22, 2017 17:44
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.

4 participants