-
Notifications
You must be signed in to change notification settings - Fork 77
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
Docker volumes should not trigger warning #760
Conversation
if (model.container != null && model.container.volumes != null && | ||
model.instances > instances | ||
) { | ||
if (model.container != null && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also fix the handleSuspendApp
condition, and maybe move the persistent volume app condition into a util.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing. I also realised we should still show the warning if the volume has .external
defined. Force-pushed some changes.
8a4a3fc
to
c804e4a
Compare
return app.container != null && | ||
app.container.volumes != null && | ||
app.container.volumes | ||
.filter(volume => volume.persistent != null || volume.external != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use volumes.some(volume => volume.persistent != null || volume.external != null)
to check if there is at least one persistent volume.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I'll do it.
c804e4a
to
0aa186f
Compare
f368849
to
983a01b
Compare
gerebased |
Looks good to me! Let's 🚢 🇮🇹 |
This fixes mesosphere/marathon#3794