Closed Bug 1056162 Opened 6 years ago Closed 6 years ago

add bit.ly support to bmo

Categories

(bugzilla.mozilla.org :: General, defect)

Production
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: glob, Assigned: glob)

Details

Attachments

(1 file, 1 obsolete file)

add bit.ly support to bmo, to allow for generation of short buglist urls.
Attached patch 1056162_1.patch (obsolete) — — Splinter Review
- add bitly extension
- improves the layout of the buglist actions

you need a bitly account with a generic access token; plug that into the parameters in the 'advanced' section.

note: bitly requires a fqdn, so a urlbase such as http://bz/1056162/ will not work.
Attachment #8475989 - Flags: review?(dylan)
Comment on attachment 8475989 [details] [diff] [review]
1056162_1.patch

Review of attachment 8475989 [details] [diff] [review]:
-----------------------------------------------------------------

r- solely because I'm not sure the PERL_LWP_SSL_VERIFY_HOSTNAME bit is intentional.

Everything worked (after installing LWP::Protocol::https, anyway).

::: extensions/Bitly/Config.pm
@@ +8,5 @@
> +package Bugzilla::Extension::Bitly;
> +use strict;
> +
> +use constant NAME => 'Bitly';
> +use constant REQUIRED_MODULES => [];

This extension implicitly requires LWP::Protocol::https. Probably not an issue in production, I would imagine, but maybe it should be listed here?

::: extensions/Bitly/lib/WebService.pm
@@ +96,5 @@
> +        uri_escape($uri->as_string)
> +    );
> +
> +    # request
> +    $ENV{PERL_LWP_SSL_VERIFY_HOSTNAME} = 0;

Two nits here:

1) Turning off ssl verification should probably come with a comment explaining the reason.
2) Setting a global/environment variable should probably be localized:
   local $ENV{PERL_LWP_SSL_VERIFY_HOSTNAME} = 0;
Attachment #8475989 - Flags: review?(dylan) → review-
Comment on attachment 8475989 [details] [diff] [review]
1056162_1.patch

Review of attachment 8475989 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/list/list.html.tmpl
@@ -227,5 @@
>          [% END %]
> -
> -        [% IF bugowners && user.id %]
> -          <a href="mailto:
> -            [% bugowners FILTER html %]">Send&nbsp;Mail&nbsp;to&nbsp;[% terms.Bug %]&nbsp;Assignees</a> |

Release management used to use this functionality a lot, though not sure they do anymore. Might want to check with them before removing it...
(In reply to Reed Loden [:reed] from comment #3)
> Release management used to use this functionality a lot, though not sure
> they do anymore. Might want to check with them before removing it...

i've emailed release management; will add it back if they use it.
Attached patch 1056162_2.patch — — Splinter Review
- removes PERL_LWP_SSL_VERIFY_HOSTNAME
- add dependency on LWP::Protocol::https and Mozilla::CA (Mozilla::CA means
  we don't need to skip hostname verification)
- move the "REST" link from the bitly extension to the BMO extension, and tweak
  the generated query-string slightly
Attachment #8475989 - Attachment is obsolete: true
Attachment #8480400 - Flags: review?(dylan)
Comment on attachment 8480400 [details] [diff] [review]
1056162_2.patch

Review of attachment 8480400 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan
Attachment #8480400 - Flags: review?(dylan) → review+
(In reply to Byron Jones ‹:glob› from comment #4)
> (In reply to Reed Loden [:reed] from comment #3)
> > Release management used to use this functionality a lot, though not sure
> > they do anymore. Might want to check with them before removing it...
> 
> i've emailed release management; will add it back if they use it.

Didn't know about it, don't rely on it, fine with removal.
looks like Mozilla::CA isn't installed on the webheads, nor do i see it on the default RHEL repos.
i've filed bug 1061080 to get it installed.

so we're not blocked on that, the version i've committed will disable certification verification if Mozilla::CA is no installed.

i'll also set up a bit.ly account bound to the bugzilla-admin address.

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   2251b98..1f6f7f1  master -> master
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Byron Jones ‹:glob› from comment #8)
> i'll also set up a bit.ly account bound to the bugzilla-admin address.

Why not just use the MoFo one that is tied to mzl.la?
(In reply to Reed Loden [:reed] from comment #9)
> (In reply to Byron Jones ‹:glob› from comment #8)
> > i'll also set up a bit.ly account bound to the bugzilla-admin address.
> 
> Why not just use the MoFo one that is tied to mzl.la?

great question :)

it would be nice to use a bugzil.la domain (eg. list.bugzil.la) and as far as i can tell you can only have branded domain per user.  i'm not particularly attached to that idea however.

looking over the configuration of mofo's bitly user, it appears there's some bmo specific tracking enabled there.  i've emailed the owner of that bitly account as i'm not sure how this would interact should we create a separate account (ie. it may be easier to just use mzl.la here).
i've updated the extension to only depend on LWP::Protocol::https if LWP 6 is installed:

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   2a9db67..66aaf86  master -> master

we can't depend on LWP::Protocol::https if LWP 5 is installed, because it's still part of the LWP package at that time, and doesn't defined VERSION.
*sigh* proxy support:

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   66aaf86..77f0eba  master -> master
You need to log in before you can comment on or make changes to this bug.