#65 Improve user experience when testcloud fails because of missing group
Merged 3 years ago by frantisekz. Opened 3 years ago by frantisekz.

file modified
+11 -2
@@ -15,7 +15,7 @@ 

  from . import config

  from . import image

  from . import instance

- from .exceptions import TestcloudCliError

+ from .exceptions import TestcloudCliError, TestcloudPermissionsError

  

  config_data = config.get_config()

  
@@ -63,7 +63,16 @@ 

      log.debug("create instance")

  

      tc_image = image.Image(args.url)

-     tc_image.prepare()

+     try:

+         tc_image.prepare()

+     except TestcloudPermissionsError as error:

+         # User might not be part of testcloud group, print user friendly message how to fix this

+         print(error)

+         print("")

+         print("You should be able to fix this by calling following commands:")

+         print("sudo usermod -a -G testcloud $USER")

+         print("su - $USER")

+         sys.exit(1)

  

      existing_instance = instance.find_instance(args.name, image=tc_image,

                                                 connection=args.connection)

file modified
+3
@@ -22,6 +22,9 @@ 

      """Exception for errors having to do with images and image fetching"""

      pass

  

+ class TestcloudPermissionsError(TestcloudException):

+     """Exception for errors from insufficient permissions on the file system."""

+     pass

  

  class TestcloudInstanceError(TestcloudException):

      """Exception for errors having to do with instances and instance prep"""

file modified
+2 -2
@@ -17,7 +17,7 @@ 

  import requests

  

  from . import config

- from .exceptions import TestcloudImageError

+ from .exceptions import TestcloudImageError, TestcloudPermissionsError

  

  config_data = config.get_config()

  
@@ -155,7 +155,7 @@ 

  

          except OSError:

              # note: suppress inside exception warnings

-             raise TestcloudImageError(

+             raise TestcloudPermissionsError(

So how do you know that you this wasn't a download/network error? Looking at the code, any network error will now be reported as TestcloudPermissionsError and you'll claim that the user can fix their network errors by being in a correct group.

                  'Problem writing to {}. Are you in group testcloud?'.format(local_path)

              ) from None

  

file modified
+1 -1
@@ -533,7 +533,7 @@ 

              else:

                  raise TestcloudInstanceError(

                      "Cannot remove running instance {}. Please stop the "

-                     "instance before removing.".format(self.name))

+                     "instance before removing or use '-f' parameter.".format(self.name))

  

          # remove from libvirt, assuming that it's stopped already

          if domain_state is not None:

no initial comment

rebased onto 296428ad62af739f1a7529c01500540c345c9e6b

3 years ago

@psss @mvadkert Are you okay with this change? It affects behavior of _create_instance() from testcloud.cli .

Can you fix all TestcloudImageErrors by changing a group? This might lead to an opposite effect, where testcloud claims something when the root cause and a fix is completely different, confusing the user once again. It would make sense to either test the access rights and print the error only when they're missing, or say something like "You might not have enough privileges... Please make sure that you're part of the 'testcloud' group by running... and log out and back in after that"

and log out and back in after that"

You don't need to do that.

I'll reply to the rest of the comment later.

rebased onto 5ed6f5eaf363d251342b67098acccc122eaaa882

3 years ago

I've created a new exception only for permission errors @kparal .

Of course, if you install testcloud from git repository, you need to create folders with proper owners, but I can take a look at that part later.

You don't need to do that.

Are you sure that you know exactly what newgrp does? All files that get created in that session will have the new group instead of your default user group. Try touch file. You'll lose access to your fuse mounts. And probably more. I'd rather recommend logging out and back in, or doing su - $USER in that particular terminal.

I would rephrase to:

Exception for errors from insufficient permissions on the file system.

Or something like that :)

Otherwise seems good to me.

As for our tmt usage, maybe it would be a good idea to catch this new exception here:

https://github.com/psss/tmt/blob/master/tmt/steps/provision/testcloud.py#L392

What do you think @psss? But we currently do not handle even the old error ...

rebased onto 7a1c2a906e696da551e7b663574d7dedb392d5ae

3 years ago

Feedback should be addressed.

As for our tmt usage, maybe it would be a good idea to catch this new exception... What do you think @psss? But we currently do not handle even the old error ...

Yeah, makes sense to catch the exception there. But we store images in our custom directory so the messaging has to be different. I've outlined the change in the following pull request:

https://github.com/psss/tmt/pull/398

So how do you know that you this wasn't a download/network error? Looking at the code, any network error will now be reported as TestcloudPermissionsError and you'll claim that the user can fix their network errors by being in a correct group.

So how do you know that you this wasn't a download/network error? Looking at the code, any network error will now be reported as TestcloudPermissionsError and you'll claim that the user can fix their network errors by being in a correct group.

Testcloud claimed that even before this PR ("Problem writing to {}. Are you in group testcloud?") ...

I can fix that in later PR together with using some standard library for downloading (urllib). This is out of scope of this PR.

The difference is that before it was a suggestion of a potential problem. Now you claim that you can definitely fix this problem by being in a group. That's why I proposed a different wording. Your comment is actually fine ("User might not be part of testcloud group"), but the message you show to the user is not ("You can fix this by").

rebased onto 9899202

3 years ago

I've changed wording a little bit, if you prefer it this way.

Also... it doesn't seem you can get TestcloudPermissionsError from download/network .

It either gets stuck forever (if the download starts and then you lose network), or raise requests.exceptions.ConnectionError if you attempt to download file without network connection.

I do not have any more comments.

@psss thanks for the tmt PR

I feel I haven't managed to explain my concerns well, but I guess... ack.

Pull-Request has been merged by frantisekz

3 years ago