search

dnephin / dobi

A build automation tool for Docker applications
https://dnephin.github.io/dobi/
Apache License 2.0
309 stars 36 forks source link

Fix issue #133 #140

Closed stumoss closed 4 years ago

stumoss commented 6 years ago

Fix compose:down not dealing with dependencies correctly.

CLAassistant commented 6 years ago

CLA assistant check
All committers have signed the CLA.

stumoss commented 6 years ago

I'm not overly familiar with the code but this seems to work for the use case in the bug report.

Hopefully the fix is as simple as it looks but I'm happy to look further if there are any concerns with the proposed fix.

dnephin commented 6 years ago

Thanks for the contribution!

Unfortunately I don't think the fix is this simple. The down action (like other "remove" actions) shouldn't run the default "create" action of the dependencies, which is what will happen if the dependencies are added like this. An example is a compose resource that depends on an image. We shouldn't rebuild the image just to compose:down.

I think the problem is that the env resources is not like other resources. It's needed to populate some values so that compose:down is able to run. This ties into the problem described in #73.

I proposed a workaround in https://github.com/dnephin/dobi/issues/133#issuecomment-402331701

I think the solution might be to declare env dependencies differently, so that they will apply to all actions.

stumoss commented 6 years ago

I did think it must be more difficult than that. Thanks for the extra info though. I will try to understand it in more depth and see what I can come up with.

Do you want me to close this PR and I can comment in the issue (before making any changes) if I find an alternative approach that I think will work and then we can go from there?

dnephin commented 6 years ago

I'm fine keeping the PR open. There are so few it's easy to manage.

stumoss commented 4 years ago

Closing as I no longer use Dobi and don't have the time to work on this.