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

Difficult to change working directory in remote block #164

Closed
justindotpub opened this issue Aug 29, 2017 · 7 comments · Fixed by #174
Closed

Difficult to change working directory in remote block #164

justindotpub opened this issue Aug 29, 2017 · 7 comments · Fixed by #174
Labels
Milestone

Comments

@justindotpub
Copy link

I'm trying to write a custom task that has to cd into different directories and run commands. It does something like the following.

remote :build do
  "cd apps/myapp_web/assets && yarn install"
  "cd apps/myapp_web/assets && ./node_modules/brunch/bin/brunch b -p"
  "cd apps/myapp_web && mkdir priv/static && MIX_ENV=#{mix_env} mix phx.digest"
end

The problem I'm running into is that these commands are prefixed with cd /path/to/workspace && /usr/bin/env, so after the cd completes I'm back to my original context.

$ pwd
/tmp
$ env cd dir1 && pwd
/tmp

I can't wrap my entire command in quotes since env will say it can't find the command (thinking the whole string is my command path). The only way I can think of to deal with this with the current code is to explicitly invoke my shell and pass in the commands, like bash -c "cd ... && ...". Alternatively perhaps the remote block could take a custom directory to cd into within the build workspace? I'm not really sure what is ideal but I thought I'd provide the feedback anyway in case it is helpful.

@holetse
Copy link
Member

holetse commented Aug 29, 2017

Hmm, yeah that's a little tricky. For the phoenix digest case, have you looked into https://github.com/labzero/bootleg_phoenix? Granted you'll still probably have some issues as umbrella apps aren't really supported yet. We'd love to hear where it falls down!

More generally, we are a little hesitant to add more options to remote, as its already pretty overloaded by the attribute filter support. It could be done with a second Keyword argument, but you'd need to call it like remote :build, [], workspace: "apps/myapp_web/assets" if you weren't doing filtering. I can definitely see the value in changing the workspace location. Maybe a workspace DSL verb that changes the workspace for all nested commands? So you'd do something like:

workspace "apps/myapp_web/assets" do
  remote :build do
    "yarn install"
    "./node_modules/brunch/bin/brunch b -p"
  end
end

Alternately we could do create a breaking change where filter attributes are part of an options Keyword, like remote :build, filter: [foo: :bar], workspace: "apps/myapp_web/assets". Of those three ideas, what would you find to be most intuitive?

While we work on a proper solution, you could create a new role that has the same details as your build role but a different workspace. Or upload a bash script to the server with your commands in it via upload.

@justindotpub
Copy link
Author

justindotpub commented Aug 29, 2017

bootleg_phoenix doesn't work with the latest bootleg or umbrella projects or phoenix 1.3. Looking into contributing to it is on my todo list. 😄

I'm not following the filter option. Can you elaborate?

The thing I thought of initially, simply due to exposure to it, was a :cd option, like that used for Phoenix watchers configuration below.

config :myapp_web, MyappWeb.Endpoint,
  watchers: [node: ["node_modules/brunch/bin/brunch", "watch", "--stdin",
                    cd: Path.expand("../assets", __DIR__)]]

That would obviously collide with the concept of a workspace, unless there was a clean way to enforce that it must be a relative path of workspace, in which case I'd imagine something like...

remote :build, cd: "apps/myapp_web/assets" do
  "yarn install"
  "./node_modules/brunch/bin/brunch b -p"
end

@holetse
Copy link
Member

holetse commented Aug 29, 2017

bootleg_phoenix would love your contributions! 😄

Currently remote lets you pass in attributes to filter which hosts in the role to use. You can find details at https://hexdocs.pm/bootleg/Bootleg.Config.html#remote/3. That's done using a Keyword list. In your example above, we'd currently interpret the cd as "all hosts in the build role which have an attribute of cd and a value "apps/myapp_web/assets". Adding extra config options to remote would need a second Keyword list as an argument or a breaking change to how filtering works. This sort of filtering is particularly useful for migrations, which should only be run on a single node in most cases. The more I think about it, a breaking change might be best (@rjanja, @brienw thoughts?). Something like:

remote :build, cd: "apps/myapp_web/assets", filter: [foo: :bar] do
  "yarn install"
  "./node_modules/brunch/bin/brunch b -p"
end

As for relative vs. absolute, it'd be simple enough to say that relative is relative to the workspace, and absolute is absolute. We use the same rules for workspace cleaning.

@rjanja
Copy link
Contributor

rjanja commented Aug 29, 2017

I like the idea of needing to be specific about filters to apply, so user-defined filters are uniquely named and separate from both current and future Bootleg options (e.g. cd).

The current behavior may somewhat limit our ability to add options in the future without potentially colliding with users' role options, so a breaking change might be better made now than later.

The more I look at that suggestion the more I like it 👍 . We could also detect an improper use based on the absence of known option keys like cd and filter to safely abort execution.

@justindotpub
Copy link
Author

Ah, I see what you're saying now. Yeah, I'd vote for the breaking change. Flexible options for this and other things down the road seems like a better way to go.

@holetse holetse added this to the 0.5.0 milestone Aug 29, 2017
@justindotpub
Copy link
Author

If the filter is just for roles maybe call it role_filter instead to be clear?

@rjanja
Copy link
Contributor

rjanja commented Sep 1, 2017

This is probably too much work to do before the next release, so we might just update the docs to remove the mention of it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants