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

fix: execute commands using correct stack path #16

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

katcipis
Copy link
Contributor

@katcipis katcipis commented Nov 8, 2021

Fixes: #14

Usually I like to add automated tests, but there isn't anything for the run function, and since we want to already refactor the design, it seems desireable to move run to be part of the core logic, not on cmd/main (like it was done with list/list changed).

Usually I would also refactor first + fix the bug on the new improved design, but since there is no tests the refactoring itself may break some features and we want a stable initial v0.0.1 ASAP.

So I did the unusual surgery mode fix. But refactoring around "run" should come ASAP.

@katcipis katcipis self-assigned this Nov 8, 2021
@katcipis
Copy link
Contributor Author

katcipis commented Nov 8, 2021

The current fix makes it more clear where commands will be run by using absolute paths, but it does introduce some inconsistency with how things are show on terrastack list, this modded version of the test script shows the difference:

#!/bin/bash

set -o errexit
set -o nounset

stack_absolute_path=$(mktemp -d)

cd "${stack_absolute_path}"

mkdir stack1
touch stack1/terrastack

mkdir stack2
touch stack2/terrastack

echo
echo "=== RUNNING INSIDE STACK BASE DIR ==="
# Running with absolute path basedir, inside stack, works:
terrastack run -b "${stack_absolute_path}" ls

echo "terrastack list"
terrastack list

echo
echo "=== RUNNING OUTSIDE STACK BASE DIR ==="
# Running with absolute path basedir, outside stack, breaks:
cd ..
terrastack run -b "${stack_absolute_path}" ls

echo "terrastack list"
terrastack list "${stack_absolute_path}"

Outputs:

=== RUNNING INSIDE STACK BASE DIR ===
Running on all stacks:
[/tmp/tmp.M0hXsYRJ3i/stack1] running /bin/ls
terrastack

[/tmp/tmp.M0hXsYRJ3i/stack2] running /bin/ls
terrastack

terrastack list
stack1
stack2

=== RUNNING OUTSIDE STACK BASE DIR ===
Running on all stacks:
[/tmp/tmp.M0hXsYRJ3i/stack1] running /bin/ls
terrastack

[/tmp/tmp.M0hXsYRJ3i/stack2] running /bin/ls
terrastack

terrastack list
tmp.M0hXsYRJ3i/stack1
tmp.M0hXsYRJ3i/stack2

Specially when running commands is involved, I kinda liked being able to see the absolute path + the code is easier to reason with (commands are run on a absolute path and that is it). But it does introduce an inconsistency with listing output.

For the v0.0.1 should we push with this or do further changes considering the PWD to replicate the list behavior ?

WDYT @mariux @i4ki ?

@katcipis katcipis requested review from i4ki and mariux November 8, 2021 16:51
@katcipis katcipis changed the title fix: execute commands using stack absolute path fix: execute commands using correct stack path Nov 8, 2021
@katcipis katcipis marked this pull request as ready for review November 8, 2021 16:57
@i4ki
Copy link
Contributor

i4ki commented Nov 8, 2021

I don't see this as an inconsistency because run needs to set its working directory to the stack directory, then if there's any permission error Go is going to report an absolute path in the error, and this will be very inconsistent.

So I vote to make the terrastack run show absolute paths but all list variants use relative by default.

Copy link
Contributor

@i4ki i4ki left a comment

Choose a reason for hiding this comment

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

LGTM

@katcipis katcipis merged commit cb034d0 into main Nov 9, 2021
@katcipis katcipis deleted the katcipis/bugfix-issue-14 branch November 9, 2021 10:54
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.

bug: running commands on stacks from outside dir breaks
2 participants