Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

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

Closed
wants to merge 1 commit into from

Conversation

ulyssessouza
Copy link
Contributor

@ulyssessouza ulyssessouza commented Nov 13, 2018

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
Copy link
Member

Awesome! And this would resolve #396. 😄

@ulyssessouza ulyssessouza changed the title Add --disable flag to render command WIP - Fixes #396 Add --disable flag to render command Nov 13, 2018
@codecov
Copy link

codecov bot commented Nov 13, 2018

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%) ⬇️
render/render.go 73.25% <0%> (-8.13%) ⬇️
internal/helm/helm.go 52.8% <0%> (-3.22%) ⬇️
types/types.go 88.59% <0%> (-2.11%) ⬇️
types/settings/merge.go 100% <0%> (ø) ⬆️
cmd/docker-app/renderfile.go
cmd/docker-app/render.go 76.66% <0%> (+1.66%) ⬆️
cmd/docker-app/root.go 77.35% <0%> (+2.78%) ⬆️
cmd/docker-app/deploy.go 32.6% <0%> (+11.6%) ⬆️
types/settings/settings.go 96.82% <0%> (+15.43%) ⬆️

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
Copy link
Member

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

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

I think you can add a WithDisabledService function, like WithName and add disabledServices to the app type, so you can remove it from the Render signature.

@ulyssessouza ulyssessouza force-pushed the 396-disable-flag branch 2 times, most recently from f58e743 to 4978042 Compare November 21, 2018 22:54
Adds a set/map of DisabledServices to App so they can be ignored by the render command.

Signed-off-by: Ulysses Souza <ulyssessouza@docker.com>
@ulyssessouza ulyssessouza changed the title WIP - Fixes #396 Add --disable flag to render command WIP - Add --disable flag to render command Nov 21, 2018
@ulyssessouza
Copy link
Contributor Author

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

@ulyssessouza ulyssessouza self-assigned this Nov 23, 2018
@mikesir87
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Member

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

@silvin-lubecki
Copy link
Contributor

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
Copy link
Member

Makes sense. Thanks @silvin-lubecki 👍

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

Successfully merging this pull request may close these issues.

Ability to disable service via flag
5 participants