Reduce permissions for pubsub #273
Conversation
| System.err.println("Subscription not found " + subscriptionName); | ||
| throw new RuntimeException(e.toString()); | ||
| } catch (PermissionDeniedException e) { | ||
| System.err.println("Missing permissions to access subscription " + subscriptionName); |
There was a problem hiding this comment.
What's wrong with the generic messages? I'd like to understand specifically what the difference is since I really try to avoid using the exception mechanism to specifically detail logging (it just ends up being messy in the long run, and I'm not sure it's the "right" place for it). All Exceptions can be of the form (string_message, exception_thrown), which keeps the relevant detail.
Are you trying to make the stack-trace disappear? Or is it really a question of the message delivered?
There was a problem hiding this comment.
I wanted to add a user friendly Missing permissions to access subscription message because it's a common error which is buried in the stack trace that people often can't read.
But I think I went about it the wrong way, what I should have done was:
throw new RuntimeException("Missing permissions to access subscription " + subscriptionName, e)
?
There was a problem hiding this comment.
So yes, there's two separable things here... 1) the error message, which the fix you indicated will address. and then 2) getting buried in the stack trace. The thing we should do for 2) is to figure out where it's being caught at the top, and introduce a "verbose" mode or something so that it only prints out the stack if verbose is enabled, otherwise it just shows the messages from the call chain. If you want to take a stab at that go for it, otherwise I can put the skeleton in place and then you can tweak the messages themselves...
There was a problem hiding this comment.
Could we move traceback output to DEBUG level and then change the log level with an --option?
|
Updated to take out changes to logging - it's now functionally equivalent just without the need for the |
Remove requirement for project.subscription.list IAM permissions to use validator by using the
NOT_FOUNDexception. Slightly improve logging (add "no permission" message line)