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

feature: services built from a Dockerfile in parent path #44

Merged
merged 4 commits into from
Nov 6, 2018

Conversation

mpareja
Copy link
Member

@mpareja mpareja commented Nov 5, 2018

I've got to hand it to you @darrenmce, the bats tests helped me avoid a regression. Adding a new docker workspace to test out builds was great too!

I need to figure out how to run the tests locally so I don't clumsily bumble my way through setting up the tests correctly.

@mpareja
Copy link
Member Author

mpareja commented Nov 5, 2018

My particular use case had a relative path to the dockerfile (../yada). I'll double check that the tests break without my changes. If they dont, I'll update test fixture to encompass my test case.

@mpareja mpareja changed the title feature: support services built from a Dockerfile feature: support services built from a Dockerfile in parent relative path Nov 5, 2018
@mpareja mpareja changed the title feature: support services built from a Dockerfile in parent relative path feature: services built from a Dockerfile in parent path Nov 5, 2018
@mpareja
Copy link
Member Author

mpareja commented Nov 5, 2018

Okay, I've updated the test to use a relative path above the directory with the docker-compose file. That's my use case. I have a repository with a bunch of pieces that I then stitch together using Tarantino.

directory structure:

- something
- tarantino
  - docker-compose
  - other-local-specific-config-files
- otherthing

The docker-compose file builds the something and otherthing directories using relative paths.

@@ -13,7 +13,7 @@ get_tt_env() {
}

dc() {
env $(get_tt_env) docker-compose -f $(dc_file) $*
env $(get_tt_env) docker-compose -p $(get_workspace) -f $(dc_file) $*
Copy link
Member

Choose a reason for hiding this comment

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

this will fix tt workspace add foo /tmp/bar making containers under the name bar rather than foo?

this is a big one! enforcing sanitised directory names sucks, but we can enforce alias names! (eventually)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the container names end up based on the workspace name (which is nice). I think your example is backward, though.

It does, however, mean you'll need to destroy and recreate workspace containers. Might need to do that before upgrading, hmm.

Copy link
Member

Choose a reason for hiding this comment

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

yeah meh breaking change is fine, folder and ws is the same name usually anyway

I worded my comment weird, (it's fixing that backwards behavior) but yeah that's what I meant

test/test_helper/workspace.bash Outdated Show resolved Hide resolved
test/test_helper/workspace.bash Show resolved Hide resolved
test/test_yamls/build/tool.sh Outdated Show resolved Hide resolved
test/test_yamls/build/Dockerfile Outdated Show resolved Hide resolved
@darrenmce
Copy link
Member

This is awesome, I'm starting to really dig project local tarantino folders

you hid a huge fix in your PR with the -p flag on the dc

@darrenmce
Copy link
Member

darrenmce commented Nov 5, 2018

also my command for running tests locally is, maybe we can add this in to the repo somehow - cuz yeah running the tests actually locally is 😭

docker run --rm -it \
  -v "$PWD:/tt" -v "/var/run/docker.sock:/var/run/docker.sock:ro" -w=/tt \
  darrenmce/tt-test-base \
  sh -c "sudo make install && DEBUG=true bats test/tt_ip.bats; cat debug.log"

also dont forget to install submodules first

git submodule sync
git submodule update --init

darrenmce and others added 3 commits November 5, 2018 19:55
Co-Authored-By: mpareja <mpareja@360incentives.com>
Co-Authored-By: mpareja <mpareja@360incentives.com>
Co-Authored-By: mpareja <mpareja@360incentives.com>
BUILD_SERVICE_DIR="${PWD}/test/test_yamls/build"

function setup() {
add_dir_with_ws ${BUILD_SERVICE_DIR} tarantino/build_service.yml
Copy link
Member

Choose a reason for hiding this comment

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

Ah didn't notice you actually needed that subfolder on the yaml file... yeah remove that last commit I suggested and go for the merge

Copy link
Member Author

Choose a reason for hiding this comment

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

This new GitHub feature makes hitting apply from mobile too easy. Going to need to be more responsible in the future. I think the last four commits were probably the worst commit messages I've ever written ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Now, if only I could do a mobile revert. Hahaha!

Copy link
Member

Choose a reason for hiding this comment

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

haha they actually go down as my commits, yeah it's a cool feature but probably needs a bit more meat to be fully useful (you also can't suggest anything to replace multiple lines - single only)

lol I did the suggest from mobile too.. we are both locked out until we get to desktop

@darrenmce
Copy link
Member

removed the last commit, gunna merge after checks pass

@darrenmce darrenmce merged commit 8e985d5 into master Nov 6, 2018
@darrenmce darrenmce deleted the support-build branch November 6, 2018 14:20
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.

2 participants