docker / app

Make your Docker Compose applications reusable, and share them on Docker Hub
Apache License 2.0
1.57k stars 177 forks source link

WIP - Add --disable flag to render command #418

Closed ulyssessouza closed 5 years ago

ulyssessouza commented 5 years ago

Adds a set/map with the services to be ignored by the render command. All other usages of render.Render() just pass 'nil'.

Note that it's a WIP PR. Maybe we could use a better name for the flag and use the concept of filtering for other concepts.

Closes #396

- What I did Added the capability to remove services from the render by specifying them in the command line.

- How I did it Added a new flag in the "render" command called "disable"

- How to verify it Given an already existing application package that contains 3 services called "service1", "service2" and "service3" run:

docker-app render --disable service1 --disable service2

Only "service3" should be rendered

- Description for the changelog Add a flag to the render command and pass the service names collected through the render.Render to be filtered afterwards

cat-ignore-deprecation-warnings

mikesir87 commented 5 years ago

Awesome! And this would resolve #396. ๐Ÿ˜„

codecov[bot] commented 5 years ago

Codecov Report

Merging #418 into master will decrease coverage by 1.85%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #418      +/-   ##
==========================================
- Coverage    60.4%   58.55%   -1.86%     
==========================================
  Files          60       59       -1     
  Lines        3791     3378     -413     
==========================================
- Hits         2290     1978     -312     
+ Misses       1242     1140     -102     
- Partials      259      260       +1
Impacted Files Coverage ฮ”
pkg/yatee/yatee.go 68.5% <0%> (-11.13%) :arrow_down:
render/render.go 73.25% <0%> (-8.13%) :arrow_down:
internal/helm/helm.go 52.8% <0%> (-3.22%) :arrow_down:
types/types.go 88.59% <0%> (-2.11%) :arrow_down:
types/settings/merge.go 100% <0%> (รธ) :arrow_up:
cmd/docker-app/renderfile.go
cmd/docker-app/render.go 76.66% <0%> (+1.66%) :arrow_up:
cmd/docker-app/root.go 77.35% <0%> (+2.78%) :arrow_up:
cmd/docker-app/deploy.go 32.6% <0%> (+11.6%) :arrow_up:
types/settings/settings.go 96.82% <0%> (+15.43%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more ฮ” = absolute <relative> (impact), รธ = not affected, ? = missing data Powered by Codecov. Last update 4b3bef0...a4015dd. Read the comment docs.

mikesir87 commented 5 years ago

Any word on this one? If this gets merged, itโ€™ll tweak and simplify some of my DockerCon talk (for the better).

ulyssessouza commented 5 years ago

Just added a test and refactored by adding a WithDisabledService as suggested by @silvin-lubecki

mikesir87 commented 5 years ago

Sorry to keep poking, but wondering if this might be merged/released before DockerCon (I know there are probably a ton of other prep stuff you're all working on). We have a tool that builds on top of Docker App (mikesir87/devdock-node) that is using the old x-enabled method to disable services. Would be much simpler to use this approach, so would prefer to show off this method. But, would need time to update the tool and slides.

chris-crone commented 5 years ago

@mikesir87 Unfortunately we don't want to add any new features just before DockerCon so this is on hold. Looking forward to meeting you there in person!

mikesir87 commented 5 years ago

@chris-crone Ok! I'll go ahead and plan my slides using what currently exists, but then hint that a better way is in the works. Looking forward to seeing you there too! ๐ŸŽ‰

mikesir87 commented 5 years ago

Been a while since I've checked in here... any chance this might make it or will this feature just get dropped?

silvin-lubecki commented 5 years ago

Hello @mikesir87 , thanks for pinging us on this PR ๐Ÿ‘ . We are actively investigating this use case, along with the one blocking parametrization on image name. We need more time as we are looking for a generic solution and I don't think this PR will fit with it. That's why I think it makes more sense to close this PR for now.

mikesir87 commented 5 years ago

Makes sense. Thanks @silvin-lubecki ๐Ÿ‘