feat(image): add the ability to force remove an image (#497) #562

Filirom1 · 2017-02-04T14:32:39Z

Hello,

This MR implements #497

This is my first contribution on this project, I am volonteer to continue on #354, if this one is OK for you.

Cheers
Romain

Close #497

deviantony · 2017-02-11T23:28:04Z

@ncresswell do we still want to add the ability to force remove an image?

ncresswell · 2017-02-11T23:45:34Z

Im not comfortable having that easily accessible in the UI, so if we do have it, it needs to be behind a double confirmation prompt. something like "are you sure you want to force the removal of image xyz; if there are any containers dependent on this image, they will no longer function. Tick the box to acknowledge and then click continue" Rgds, Neil Cresswell On 12/02/2017, at 12:28 PM, Anthony Lapenna <notifications@github.com<mailto:notifications@github.com>> wrote: @ncresswell<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ncresswell&d=DQMCaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=0fx0h4vB56iTLpw2McH1ZD6TqG_QGpbggVOB-PfMJpM&m=n6spIBegXfFfxZF2CRuci5yAnfj7wia7Jbyoy0KreU4&s=SO9BPUzSZ_3E6TYCfbUs14ECuZO5UcNFxIHw7-G6sHM&e=> do we still want to add the ability to force remove an image? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_portainer_portainer_pull_562-23issuecomment-2D279183609&d=DQMCaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=0fx0h4vB56iTLpw2McH1ZD6TqG_QGpbggVOB-PfMJpM&m=n6spIBegXfFfxZF2CRuci5yAnfj7wia7Jbyoy0KreU4&s=KnRpMrbchGOCWIDvYEMLPl_4Buil_qoCtwNuTim-otE&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AWGrlQXYbcEHSouhAm-2Dwe1pX6jEBluy-2Dks5rbkQGgaJpZM4L3KkK&d=DQMCaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=0fx0h4vB56iTLpw2McH1ZD6TqG_QGpbggVOB-PfMJpM&m=n6spIBegXfFfxZF2CRuci5yAnfj7wia7Jbyoy0KreU4&s=WWVE62ShReSVL25ZroiIUAVGWbadbx9NOmuzhJJuzoE&e=>.

deviantony · 2017-02-12T01:58:40Z

@Filirom1 would you be able to implement that information ? I'm not a great fond of popups as I've think they're bad for UX but nonetheless I agree with Neil that some confirmation should be asked.

ncresswell · 2017-02-12T02:46:50Z

Doesnt need to be a popup, can just be an inline confirmation... ie when selecting, something becomes visible in the delete page. I also hate popups. Rgds, Neil Cresswell On 12/02/2017, at 2:58 PM, Anthony Lapenna <notifications@github.com<mailto:notifications@github.com>> wrote: @Filirom1<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Filirom1&d=DQMCaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=0fx0h4vB56iTLpw2McH1ZD6TqG_QGpbggVOB-PfMJpM&m=35N5fOJwRA1mjDh__tZy_TOld9qho7WAtUOx_0KsFb8&s=svmPy3xV5cx8SQuzPyMWxawohDQjrMzgp5-ua7NOVGc&e=> would you be able to implement that information ? I'm not a great fond of popups as I've think they're bad for UX but nonetheless I agree with Neil that some confirmation should be asked. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_portainer_portainer_pull_562-23issuecomment-2D279190383&d=DQMCaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=0fx0h4vB56iTLpw2McH1ZD6TqG_QGpbggVOB-PfMJpM&m=35N5fOJwRA1mjDh__tZy_TOld9qho7WAtUOx_0KsFb8&s=fdRJubmAaIR42lStzxkZWvZN_b4QbMg_7EfHN5KctN0&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AWGrlSzJUx4-5FezHwJA8rzXeuqJEnhJdgks5rbmdTgaJpZM4L3KkK&d=DQMCaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=0fx0h4vB56iTLpw2McH1ZD6TqG_QGpbggVOB-PfMJpM&m=35N5fOJwRA1mjDh__tZy_TOld9qho7WAtUOx_0KsFb8&s=QRXVvctbLS6hct_TtlRZ7TjFPIboMazVGByHZGdMCUo&e=>.

Filirom1 · 2017-02-13T08:41:43Z

Hi @ncresswell

I don't see what you are asking for. If you have examples it could help me.

Otherwize, with html5 dialog we could have something like this easily:

Pressing the ESC on the keyboard also cancel the dialog.

As for I as know, docker rmi -f doesn't break running container, it only removes tags on the image, but docker rmi -f is needed to remove an image with several tags.

ncresswell · 2017-02-13T09:22:03Z

That html5 dialog box works for me... something that forces the user to confirm that they acknowledge the risk and want to continue anyway Rgds, Neil Cresswell On 13/02/2017, at 9:41 PM, Romain <notifications@github.com<mailto:notifications@github.com>> wrote: Hi @ncresswell<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ncresswell&d=DQMFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=0fx0h4vB56iTLpw2McH1ZD6TqG_QGpbggVOB-PfMJpM&m=UrLpPBEbxQ1KdzAeveCmbWTYOGT1cPBhgfYwTwx6kIo&s=yAMQ_BGdWrcvV9LdKWRAfjjKUMQgkv1FdUE8U9NZoio&e=> I don't see what you are asking for. If you have examples it could help me. Otherwize, with html5 dialog we could have somtheing like easily: [image]<https://urldefense.proofpoint.com/v2/url?u=https-3A__cloud.githubusercontent.com_assets_295554_22876266_19a7289e-2Df1d0-2D11e6-2D862d-2Dcb9aa897981a.png&d=DQMFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=0fx0h4vB56iTLpw2McH1ZD6TqG_QGpbggVOB-PfMJpM&m=UrLpPBEbxQ1KdzAeveCmbWTYOGT1cPBhgfYwTwx6kIo&s=cJIf4_UM9V1hPWpQNEO_nu0bRQoCbfOeqvMpJEHHPaA&e=> Pressing the ESC on the keyboard also cancel the dialog. As for I as know, docker rmi -f doesn't break running container, it only removes tags on the image, but docker rmi -f is needed to remove an image with several tags. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_portainer_portainer_pull_562-23issuecomment-2D279324944&d=DQMFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=0fx0h4vB56iTLpw2McH1ZD6TqG_QGpbggVOB-PfMJpM&m=UrLpPBEbxQ1KdzAeveCmbWTYOGT1cPBhgfYwTwx6kIo&s=xdXa1Xdmn3tDdqUC4czoVwJsp5mpdhaqDkjik7RZMKQ&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AWGrleULX4TG03YZXKN0-5FX9-5FCe7-2DEAk5ks5rcBdJgaJpZM4L3KkK&d=DQMFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=0fx0h4vB56iTLpw2McH1ZD6TqG_QGpbggVOB-PfMJpM&m=UrLpPBEbxQ1KdzAeveCmbWTYOGT1cPBhgfYwTwx6kIo&s=qqR-ESylZxqY_8MxYiP1mRnJNOf_BP2WXRIviefqnes&e=>.

deviantony · 2017-02-13T09:23:58Z

IMO, message needs to be udpated as a user could select multiple images and trigger the force remove.
I'll give a try to the HTML5 box to see if it fits well with Portainer design.

Filirom1 · 2017-02-13T09:44:44Z

Ok, I will push my commits once I get back home.

And we will iterate on it to have something great :-)

deviantony · 2017-02-13T09:45:45Z

Perfect ! Thanks a lot @Filirom1 👍

deviantony · 2017-02-13T21:19:40Z

Looks like something went wrong ;)

You should just pull/merge develop instead of doing a rebase.

Filirom1 · 2017-02-13T21:23:51Z

Yes, I rebase on master. Bad idea 😢

deviantony · 2017-02-13T21:25:27Z

That's better ! I'll have a look at it today.

Filirom1 · 2017-02-13T21:26:59Z

HTML 5.1 dialog are not well supported by browsers.
Finally I switched to bootstrap modals.

Are you OK with the usage of the bootbox library ? It eases modal usage.

deviantony · 2017-02-14T08:42:06Z

Even if I despise popups, I do not see any alternative here. I'm OK for the bootbox library.

IMO, the message should be updated to "Forcing the removal of the image will remove the image even if it has multiple tags or if it is used by stopped containers".

Filirom1 · 2017-02-14T09:15:40Z

I like your message, I commit it 👯‍♂️

deviantony

Have a look at my comments for a few changes.

Also, please run grunt lint and fix linting issues.

Oh and one more thing, would it be possible to have the modal centered on the screen instead of it opening in the top part of the screen ?

deviantony · 2017-02-14T22:23:18Z

app/components/images/imagesController.js

+  $scope.confirmRemovalAction = function (force) {
+    bootbox.confirm({
+      title: "Are you sure?",
+      message: "Forcing the removal of the image will remove the image even if it has multiple tags or if it is used by stopped containers",


Could you add an extra '.' at the end of the sentence?

deviantony · 2017-02-14T22:25:06Z

app/components/images/imagesController.js

+      message: "Forcing the removal of the image will remove the image even if it has multiple tags or if it is used by stopped containers",
+      buttons: {
+        confirm: {
+          label: 'Force',


Replace the label with 'Remove the image'

deviantony · 2017-02-14T22:29:27Z

app/components/images/imagesController.js

@@ -59,7 +59,28 @@ function ($scope, $state, Config, Image, ImageHelper, Messages, Pagination) {
    });
  };

-  $scope.removeAction = function () {
+  $scope.confirmRemovalAction = function (force) {
+    bootbox.confirm({


Do you think you could isolate the bootbox call inside a specific service ? Like ModalService ? This way if we want to get rid of bootbox later, we'll just have to change the implementation there.

Filirom1

Done

Filirom1 · 2017-02-15T07:55:02Z

About the linter, I fixed my code, but the linter is still displaying a lot of errors elsewhere in the code.

Are you aware of it ?

$ grunt lint |grep ERROR
Linting gruntfile.js...ERROR
Linting app/app.js...ERROR
Linting app/components/createNetwork/createNetworkController.js...ERROR
Linting app/components/createService/createServiceController.js...ERROR
Linting app/components/images/imagesController.js...ERROR
Linting app/components/networks/networksController.js...ERROR
Linting app/components/stats/statsController.js...ERROR
Linting app/components/templates/templatesController.js...ERROR
Linting app/models/event.js...ERROR
Linting app/rest/container.js...ERROR
Linting app/rest/event.js...ERROR
Linting app/rest/exec.js...ERROR
Linting app/rest/image.js...ERROR
Linting app/rest/response/handlers.js...ERROR
Linting app/rest/version.js...ERROR
Linting app/rest/volume.js...ERROR
Linting app/services/containerService.js...ERROR
Linting app/services/imageService.js...ERROR
Linting app/services/templateService.js...ERROR
Linting app/services/volumeService.js...ERROR

deviantony · 2017-02-15T10:25:04Z

Please run npm install again to ensure that you have the latest version of the linter installed. I do not have these issues.

Updated PR

deviantony

An update is required in the ModalService.

deviantony · 2017-02-15T10:35:16Z

app/components/images/imagesController.js

@@ -59,7 +59,28 @@ function ($scope, $state, Config, Image, ImageHelper, Messages, Pagination) {
    });
  };

-  $scope.removeAction = function () {
+  $scope.confirmRemovalAction = function (force) {
+    ModalService.confirm({


I'd go farther when isolating the bootbox inside the ModalService. ModalService methods should be pretty abstract and should not know about the required fields. It should only expose a confirmationModal method with the things that can change inside a confimation modal (e.g. title, message, confirmLabel, cancelLabel and the success callback).

Something like ModalService.confirmModal('Are you sure?', Forcing the removal of the image will remove the image even if it has multiple tags or if it is used by stopped containers.', 'Remove the image', 'Cancel', function () { ... })

Hi,

I understand your point, but passing such parameters as arguments will make this module hard to evolve.

I would prefer passing parameters as an object:

ModalService.confirmModal({ title: 'Are you sure?', message: 'Forcing the removal of the image will remove the image even if it has multiple tags or if it is used by stopped containers.', confirm: { label: 'Remove the image' }, cancel: { label: 'cancel' } })

To make my point clear, in our situation, the className of the confirm button is red for danger.
This may not be the case, if later, for example, we want to use this service to confirm a password change.

Passing an object will make it simpler to extend with additional parameters.

I offer you this new solution in a5944dc, based on object passing.

I agree, this is OK for me.

deviantony · 2017-02-15T10:36:26Z

assets/css/app.css

@@ -135,6 +139,23 @@ input[type="radio"] {
    line-height: 1.42857143;
    border-radius: 4px;
  }
+


I assume this code is related to the centering of the modal. Have you tried to do it in the service directly ? See bootboxjs/bootbox#166 (comment)

Good point.

Updated PR

deviantony

LGTM

deviantony · 2017-02-15T22:14:35Z

Thanks @Filirom1 👍

Also, please update your local dev dependencies as there is still a small linting issue:

Running "jshint:files" (jshint) task

   app/services/modalService.js
     26 |  }
            ^ Missing semicolon.

>> 1 error in 94 files

I'll merge it and fix the linting issue.

Co-authored-by: Simon Meng <simon.meng@portainer.io>

deviantony added the contrib/func-review-in-progress label Feb 11, 2017

Filirom1 added 5 commits February 13, 2017 10:25

feat(image): add the ability to force remove an image (portainer#497)

8d5fe93

feat(image): ask for confirmation before force removal

e984b5f

style(image): lint and update confirmation message

0df8da6

refactor(image): move bootbox into modalService

e25ec59

refactor(image): low coupling to bootbox and handle positioning with JS

a5944dc

Filirom1 force-pushed the force-remove-image branch from bd14a76 to 9c12eba Compare February 13, 2017 21:16

Filirom1 force-pushed the force-remove-image branch from 9c12eba to e984b5f Compare February 13, 2017 21:22

Filirom1 added 2 commits February 14, 2017 10:03

refactor(image): change force-removal message

5a0007b

feat(image): place the modal at the center of the screen

d2ed3e8

deviantony added contrib/func-review-approved and removed contrib/func-review-in-progress labels Feb 14, 2017

deviantony previously requested changes Feb 14, 2017

View reviewed changes

Filirom1 commented Feb 15, 2017

View reviewed changes

deviantony previously requested changes Feb 15, 2017

View reviewed changes

deviantony approved these changes Feb 15, 2017

View reviewed changes

deviantony added contrib/tech-review-approved and removed contrib/tech-review-in-progress labels Feb 15, 2017

deviantony merged commit 64ef743 into portainer:develop Feb 15, 2017

chiptus pushed a commit that referenced this pull request Aug 13, 2021

feat(edge) EE-743 enable signature checking for edge agent (#562)

261b6e7

Co-authored-by: Simon Meng <simon.meng@portainer.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(image): add the ability to force remove an image (#497) #562

feat(image): add the ability to force remove an image (#497) #562

Filirom1 commented Feb 4, 2017 •

edited by deviantony

Loading

deviantony commented Feb 11, 2017

ncresswell commented Feb 11, 2017 via email

deviantony commented Feb 12, 2017

ncresswell commented Feb 12, 2017 via email

Filirom1 commented Feb 13, 2017 •

edited

Loading

ncresswell commented Feb 13, 2017 via email

deviantony commented Feb 13, 2017

Filirom1 commented Feb 13, 2017

deviantony commented Feb 13, 2017

deviantony commented Feb 13, 2017

Filirom1 commented Feb 13, 2017

deviantony commented Feb 13, 2017

Filirom1 commented Feb 13, 2017

deviantony commented Feb 14, 2017

Filirom1 commented Feb 14, 2017

deviantony left a comment •

edited

Loading

deviantony Feb 14, 2017

deviantony Feb 14, 2017

deviantony Feb 14, 2017

Filirom1 left a comment

Filirom1 commented Feb 15, 2017

deviantony commented Feb 15, 2017

deviantony left a comment •

edited

Loading

deviantony Feb 15, 2017

Filirom1 Feb 15, 2017

Filirom1 Feb 15, 2017

deviantony Feb 15, 2017

deviantony Feb 15, 2017

Filirom1 Feb 15, 2017

deviantony left a comment

deviantony commented Feb 15, 2017

feat(image): add the ability to force remove an image (#497) #562

feat(image): add the ability to force remove an image (#497) #562

Conversation

Filirom1 commented Feb 4, 2017 • edited by deviantony Loading

deviantony commented Feb 11, 2017

ncresswell commented Feb 11, 2017 via email

deviantony commented Feb 12, 2017

ncresswell commented Feb 12, 2017 via email

Filirom1 commented Feb 13, 2017 • edited Loading

ncresswell commented Feb 13, 2017 via email

deviantony commented Feb 13, 2017

Filirom1 commented Feb 13, 2017

deviantony commented Feb 13, 2017

deviantony commented Feb 13, 2017

Filirom1 commented Feb 13, 2017

deviantony commented Feb 13, 2017

Filirom1 commented Feb 13, 2017

deviantony commented Feb 14, 2017

Filirom1 commented Feb 14, 2017

deviantony left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Filirom1 left a comment

Choose a reason for hiding this comment

Filirom1 commented Feb 15, 2017

deviantony commented Feb 15, 2017

deviantony left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deviantony left a comment

Choose a reason for hiding this comment

deviantony commented Feb 15, 2017

Filirom1 commented Feb 4, 2017 •

edited by deviantony

Loading

Filirom1 commented Feb 13, 2017 •

edited

Loading

deviantony left a comment •

edited

Loading

deviantony left a comment •

edited

Loading