Re: mailgw argument parsing

Hi Ralf:

I just checked your commit. Sorry I didn't look at the branch you
pushed. When I said I removed my objection, I meant it was to the
removal of -C. But I also gave approval for the merge, which I
shouldn't have. I can make the changes if you agree.

So a few notes:

user_guide.txt has this change:


@@ -217,7 +217,7 @@
   <Range from 2003-01-01.00:00:00 to 2003-12-31.23:59:59>
 - "2003-04" means
   <Range from 2003-04-01.00:00:00 to 2003-04-30.23:59:59>
-
+

was removing the 2003-04 line intentional?  Looks like you also
cleaned up trailing spaces and reworked some text to be clearer as
well. Thanks, that's a signifcant improvement.

Does the usage string returned by roundup-mailgw when the source is
unrecognized need to be updated to include imaps_oauth?

    return usage(argv, _('Error: The source must be either "mailbox",'
                         ' "pop", "pops", "apop", "imap", "imaps" or'
                         ' "imaps_cram'))

When a user specifies imaps_oauth but doesn't have requests installed,
I see a logger error. Does that go the user if run interactively and
logging is set to a file in config.ini? If not, should we detect this
failure and both log it as you have, but report to the user from
roundup-mailgw?

Regarding the command line setting of oauth parameters (-IOT) Also it
looks like -I is required and -T defaults to
'https://login.microsoftonline.com/'organizations/oauth2/v2.0/token if
you are using imaps_oauth? Is this correct? If so where is the -T
default and argument format documented.

Also I am not sure that's the right place to set those options.
Specifying -I, -O, or -T don't turn on oauth. We don't have any
protocol specific command line options otherwise.

If you used:

  roundup-mailgw -I ... imaps username:ignored_password@server

that would be different in my view. (As an aside, is pops with OAuth a
thing?). As another example, adding a -P <password file> option to
roundup-mailgw so the password isn't exposed on the cli would be a
good multi-protocol cli option.

I wonder if modifying the imaps_oauth command line would be better.
Rather than -I client_id and -O directory and -T endpoint something
like:

  roundup-mailgw imaps_oauth username@client_id:server [mailbox] [https://path/to/oauth_endpoint] [oauth dir]

where oauth dir is "relative_path" or "/absolute/path". I am flexible
about the order of the optional params. Ideally we would put them in
the order of most likely to be changed.

So this example


  roundup-mailgw imaps_oauth username@client_id:server '' https:/oauth_server/path/to/oauth_endpoint "oauth dir"

 would be equivalent to:

   -I client_id
   -O 'oauth dir'
   -T https://oauth_server/path/to/oauth_endpoint

I agree it's a bit wierd for the client_id which is given to the oauth
endpoint (IIUC) to be associated with the mailbox server. Any missing
parameter is replaced with a '' or "" ("" only on windows) to make
parsing easier.

Alternatively

  roundup-mailgw imaps_oauth username@server clientid [mailbox] [https://path/to/oauth_endpoint] [oauth dir]

or

  roundup-mailgw imaps_oauth username@server clientid '' ''  /abs/path/to/-O/dir

so args[5] is the position of the -O flag.

An alternative idea is to put these options into the tracker's
config.ini file under [mailgw]? The downside of this is that we are
limited to only one oauth server, so maybe this is not a good idea.  I
am not user what the use case is for multiple oauth servers but I
don't know enough to rule them out so....

If you think the option -> cli args is a good thing I can do the
changes if you like.

In the test code I see changes froma tuple to an array:

-    def _create_mailgw(self, message, args=()):
+    def _create_mailgw(self, message, args=[]):

Might I ask why? IIUC you want an immutable default value for a method
argument.

Thoughts? Again apologies for not looking at the code.

Also I pushed a reference to the requests library to the
installation.txt doc for this feature.

--
				-- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.