Overriding Jenkins versus make-work patches

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Overriding Jenkins versus make-work patches

Brad Jorsch (Anomie)
We have an (informal?) policy that deprecation warnings shouldn't be raised
in WMF production. Thus if a core patch is going to add deprecation
warnings we have to make sure that all extensions deployed on the cluster
are updated to not trigger the warning. We can accomplish this by carefully
managing the addition of the warnings, by detecting the core version from
the extension and changing behavior, or by simply updating both core and
extension at the same time.

With Gerrit change 134827,[1] the affected extensions are Flow and
CentralAuth. For neither of these extensions do we care that
extension-master works with non-master versions of MediaWiki core, and
patches to update the extensions are ready.[2][3] So normally we'd just
merge all three at once and be done with it.

The problem is unit tests: we can't merge the core change[1] first because
Flow's unit tests will fail on the deprecation warning, and we can't merge
the Flow change[2] first because it doesn't work without the core change.
There are various ways to work around this, but all are ugly:

   1. Merge the core change over Jenkins's objections, then the Flow change
   can be merged as normal. But overriding Jenkins sucks.
   2. Split the core patch into two parts: part 1 does everything except
   add the wfDeprecated() call, while part 2 adds just the wfDeprecated() call
   and will be merged immediately after. The make-work here just to make
   Jenkins happy sucks and slightly clutters the commit history.
   3. Rewrite the Flow unit test to detect whether core has the core change
   and alter behavior accordingly. This is even more make-work than option 2
   when we're otherwise happy to just coordinate the merges.

Which ugly option do we as a development community prefer in this
situation? Personally I'd go for option 1 as the most expedient with the
fewest long-term consequences.

 [1]: https://gerrit.wikimedia.org/r/#/c/134827/
 [2]: https://gerrit.wikimedia.org/r/#/c/190023/
 [3]: https://gerrit.wikimedia.org/r/#/c/190026/


P.S. Let's not side-track this into whether the "extension master only
needs to be compatible with core master" policy for Flow and CentralAuth is
good/bad, or that situations exist where options 2 or 3 are necessary
choices (e.g. #2 when the extension fixes aren't ready yet and #3 when an
extension involved doesn't have the "master is only compatible with core
master" policy), or whether allowing core changes to be merged that cause
non-WMF-deployed extensions to raise deprecation warnings is somehow
discriminating against non-WMF-deployed extensions. Start a new thread if
you want to discuss those, please.



--
Brad Jorsch (Anomie)
Software Engineer
Wikimedia Foundation
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Bryan Davis
On Wed, Jun 17, 2015 at 10:53 AM, Brad Jorsch (Anomie)
<[hidden email]> wrote:

> We have an (informal?) policy that deprecation warnings shouldn't be raised
> in WMF production. Thus if a core patch is going to add deprecation
> warnings we have to make sure that all extensions deployed on the cluster
> are updated to not trigger the warning. We can accomplish this by carefully
> managing the addition of the warnings, by detecting the core version from
> the extension and changing behavior, or by simply updating both core and
> extension at the same time.
>
> With Gerrit change 134827,[1] the affected extensions are Flow and
> CentralAuth. For neither of these extensions do we care that
> extension-master works with non-master versions of MediaWiki core, and
> patches to update the extensions are ready.[2][3] So normally we'd just
> merge all three at once and be done with it.
>
> The problem is unit tests: we can't merge the core change[1] first because
> Flow's unit tests will fail on the deprecation warning, and we can't merge
> the Flow change[2] first because it doesn't work without the core change.
> There are various ways to work around this, but all are ugly:
>
>    1. Merge the core change over Jenkins's objections, then the Flow change
>    can be merged as normal. But overriding Jenkins sucks.
>    2. Split the core patch into two parts: part 1 does everything except
>    add the wfDeprecated() call, while part 2 adds just the wfDeprecated() call
>    and will be merged immediately after. The make-work here just to make
>    Jenkins happy sucks and slightly clutters the commit history.
>    3. Rewrite the Flow unit test to detect whether core has the core change
>    and alter behavior accordingly. This is even more make-work than option 2
>    when we're otherwise happy to just coordinate the merges.
>
> Which ugly option do we as a development community prefer in this
> situation? Personally I'd go for option 1 as the most expedient with the
> fewest long-term consequences.
>
>  [1]: https://gerrit.wikimedia.org/r/#/c/134827/
>  [2]: https://gerrit.wikimedia.org/r/#/c/190023/
>  [3]: https://gerrit.wikimedia.org/r/#/c/190026/
>
>
> P.S. Let's not side-track this into whether the "extension master only
> needs to be compatible with core master" policy for Flow and CentralAuth is
> good/bad, or that situations exist where options 2 or 3 are necessary
> choices (e.g. #2 when the extension fixes aren't ready yet and #3 when an
> extension involved doesn't have the "master is only compatible with core
> master" policy), or whether allowing core changes to be merged that cause
> non-WMF-deployed extensions to raise deprecation warnings is somehow
> discriminating against non-WMF-deployed extensions. Start a new thread if
> you want to discuss those, please.

For the similar but slightly different case of library version bumps
in composer.json and the associated mediawiki/vendor repo that holds
the Jenkins/Beta cluster/Prod realization of composer.json we use the
force past Jenkins option (#1). Both patches are put into Gerrit and
fail Jenkins tests: in core because functionality from the updated
library isn't available and in vendor because the library doesn't
match the version in core's composer.json. The vendor patch can be
forced and then the core patch retested to ensure that core is in the
right state with the updated vendor.

Bryan
--
Bryan Davis              Wikimedia Foundation    <[hidden email]>
[[m:User:BDavis_(WMF)]]  Sr Software Engineer            Boise, ID USA
irc: bd808                                        v:415.839.6885 x6855

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

S Page-3
In reply to this post by Brad Jorsch (Anomie)
#1 sounds right, Jenkins serves us, not vice versa. The core change's
commit message should reference the two required extension changes.
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Legoktm
In reply to this post by Brad Jorsch (Anomie)
On 06/17/2015 09:53 AM, Brad Jorsch (Anomie) wrote:
>    1. Merge the core change over Jenkins's objections, then the Flow change
>    can be merged as normal. But overriding Jenkins sucks.

Force-merging in a jenkins/zuul managed repository should be avoided as
much as possible, since it will cause zuul to deadlock and freeze[1].

>    2. Split the core patch into two parts: part 1 does everything except
>    add the wfDeprecated() call, while part 2 adds just the wfDeprecated() call
>    and will be merged immediately after. The make-work here just to make
>    Jenkins happy sucks and slightly clutters the commit history.

I would prefer this one.


[1] https://phabricator.wikimedia.org/T93812

-- Legoktm

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Joaquin Oltra Hernandez
Option 2 makes the most sense to me:

1. Implement new behavior
2. Change dependent extensions to use new behavior
3. Deprecate old behavior

That said Option 1 seems preferable to 3.

On Wed, Jun 17, 2015 at 10:17 PM, Legoktm <[hidden email]>
wrote:

> On 06/17/2015 09:53 AM, Brad Jorsch (Anomie) wrote:
> >    1. Merge the core change over Jenkins's objections, then the Flow
> change
> >    can be merged as normal. But overriding Jenkins sucks.
>
> Force-merging in a jenkins/zuul managed repository should be avoided as
> much as possible, since it will cause zuul to deadlock and freeze[1].
>
> >    2. Split the core patch into two parts: part 1 does everything except
> >    add the wfDeprecated() call, while part 2 adds just the
> wfDeprecated() call
> >    and will be merged immediately after. The make-work here just to make
> >    Jenkins happy sucks and slightly clutters the commit history.
>
> I would prefer this one.
>
>
> [1] https://phabricator.wikimedia.org/T93812
>
> -- Legoktm
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Stephane Bisson
I agree, #2 is a sequence of stable states. If any step goes wrong it
doesn't leave the system as a whole in a bad state.

On Thu, Jun 18, 2015 at 8:05 AM, Joaquin Oltra Hernandez <
[hidden email]> wrote:

> Option 2 makes the most sense to me:
>
> 1. Implement new behavior
> 2. Change dependent extensions to use new behavior
> 3. Deprecate old behavior
>
> That said Option 1 seems preferable to 3.
>
> On Wed, Jun 17, 2015 at 10:17 PM, Legoktm <[hidden email]>
> wrote:
>
> > On 06/17/2015 09:53 AM, Brad Jorsch (Anomie) wrote:
> > >    1. Merge the core change over Jenkins's objections, then the Flow
> > change
> > >    can be merged as normal. But overriding Jenkins sucks.
> >
> > Force-merging in a jenkins/zuul managed repository should be avoided as
> > much as possible, since it will cause zuul to deadlock and freeze[1].
> >
> > >    2. Split the core patch into two parts: part 1 does everything
> except
> > >    add the wfDeprecated() call, while part 2 adds just the
> > wfDeprecated() call
> > >    and will be merged immediately after. The make-work here just to
> make
> > >    Jenkins happy sucks and slightly clutters the commit history.
> >
> > I would prefer this one.
> >
> >
> > [1] https://phabricator.wikimedia.org/T93812
> >
> > -- Legoktm
> >
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> >
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>



--
Stephane Bisson
Wikimedia Foundation
1 416 270 3830
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Jon Robson
I also would recommend 2. We had this issue recently with some tweaks
Kaldari made to our main menu.

Given that lightning deploys happen we really should make master stable
always.

If Jenkins is barfing on this there may be other extensions out there which
will also barf. It also makes rolling back a lot easier if things do go
wrong.

It takes a bit more time to do and seems silly but really is the right way
to do this sort of thing. Smaller commits generally are better and I wish
we broke down a lot of our patch sets more (due to code review being slow I
think sometimes we tend to bundle too many things into any given patch).
On 18 Jun 2015 5:36 am, "Stephane Bisson" <[hidden email]> wrote:

> I agree, #2 is a sequence of stable states. If any step goes wrong it
> doesn't leave the system as a whole in a bad state.
>
> On Thu, Jun 18, 2015 at 8:05 AM, Joaquin Oltra Hernandez <
> [hidden email]> wrote:
>
> > Option 2 makes the most sense to me:
> >
> > 1. Implement new behavior
> > 2. Change dependent extensions to use new behavior
> > 3. Deprecate old behavior
> >
> > That said Option 1 seems preferable to 3.
> >
> > On Wed, Jun 17, 2015 at 10:17 PM, Legoktm <[hidden email]>
> > wrote:
> >
> > > On 06/17/2015 09:53 AM, Brad Jorsch (Anomie) wrote:
> > > >    1. Merge the core change over Jenkins's objections, then the Flow
> > > change
> > > >    can be merged as normal. But overriding Jenkins sucks.
> > >
> > > Force-merging in a jenkins/zuul managed repository should be avoided as
> > > much as possible, since it will cause zuul to deadlock and freeze[1].
> > >
> > > >    2. Split the core patch into two parts: part 1 does everything
> > except
> > > >    add the wfDeprecated() call, while part 2 adds just the
> > > wfDeprecated() call
> > > >    and will be merged immediately after. The make-work here just to
> > make
> > > >    Jenkins happy sucks and slightly clutters the commit history.
> > >
> > > I would prefer this one.
> > >
> > >
> > > [1] https://phabricator.wikimedia.org/T93812
> > >
> > > -- Legoktm
> > >
> > > _______________________________________________
> > > Wikitech-l mailing list
> > > [hidden email]
> > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> > >
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> >
>
>
>
> --
> Stephane Bisson
> Wikimedia Foundation
> 1 416 270 3830
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Brad Jorsch (Anomie)
On Thu, Jun 18, 2015 at 9:32 AM, Jon Robson <[hidden email]> wrote:

> Smaller commits generally are better


I'm going to call red herring here. Whether or not smaller commits are
really better, one patch that does

+ if ( detectOldInput( $input ) ) {
+     $input = upgradeInput( $input );
+     // wfDeprecated( "You used the old input style!" );
+ }

then a followup that does

-     // wfDeprecated( "You used the old input style!" );
+     wfDeprecated( "You used the old input style!" );

to be merged 1 second later[1] isn't an example.


 [1]: Assuming Jenkins gets a whole lot faster about merging stuff ;)


--
Brad Jorsch (Anomie)
Software Engineer
Wikimedia Foundation
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Antoine Musso-3
In reply to this post by Brad Jorsch (Anomie)
Hello Brad,

Thank you to have taken the time to elaborate on our IRC conversation
yesterday.

Le 17/06/2015 18:53, Brad Jorsch (Anomie) a écrit :
<snip>
>    1. Merge the core change over Jenkins's objections, then the Flow change
>    can be merged as normal. But overriding Jenkins sucks.
>
>    2. Split the core patch into two parts: part 1 does everything except
>    add the wfDeprecated() call, while part 2 adds just the wfDeprecated() call
>    and will be merged immediately after. The make-work here just to make
>    Jenkins happy sucks and slightly clutters the commit history.

Without the wfDeprecated(), the patch proposed for mediawiki/core is
back compatible and the extensions tests are passing just fine. So the
change is golden and can land.

Then we want to get rid of the old invocation style. One propose a
change that adds the wfDeprecated(). The tests run by
mediawiki-testextensions (a few dozen of extensions) are breaking.  To
have that change land, the extensions needs to be updated to use the new
style.  Since core supports the new change that is possible.

Once all extensions participating in mediawiki-testextensions are
updated, the wfDeprecated() call change pass tests and can land.


With time, more extensions would be added, that would let us keeping
them with mediawiki/core latest code.

>    3. Rewrite the Flow unit test to detect whether core has the core change
>    and alter behavior accordingly. This is even more make-work than option 2
>    when we're otherwise happy to just coordinate the merges.

If we had a core change impacting multiple extensions, that would need a
lot of work and effort.  core keeping back compatibility should be enough.

cheers,

--
Antoine "hashar" Musso


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Antoine Musso-3
In reply to this post by S Page-3
Le 17/06/2015 19:49, S Page a écrit :
> #1 sounds right, Jenkins serves us, not vice versa. The core change's
> commit message should reference the two required extension changes.

Whenever we upgrade Zuul, it will recognize in the commit summary
messages headers like "Depends-On: <gerrit-change-id>"

So the mediawiki/core change can have something like:

  Break API foo

  Depends-On: I1AE2309409   # fix for Flow

When you propose the patch to core, it will be tested with the dependent
change that is still open in Gerrit (ie it has not been merged yet).

Then you would +2 the changes. Zuul will first test the extension change
as if the core patch has been merged. If it passes it get merged if it
fails, the core change behind is rejected automatically.


That is nicknamed cross-repo dependency:
http://lists.openstack.org/pipermail/openstack-dev/2015-February/056515.html

No plan to upgrade our Zuul in the short term though ;(


--
Antoine "hashar" Musso


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Antoine Musso-3
In reply to this post by Jon Robson
Le 18/06/2015 15:32, Jon Robson a écrit :

> I also would recommend 2. We had this issue recently with some tweaks
> Kaldari made to our main menu.
>
> Given that lightning deploys happen we really should make master stable
> always.
>
> If Jenkins is barfing on this there may be other extensions out there which
> will also barf. It also makes rolling back a lot easier if things do go
> wrong.
>
> It takes a bit more time to do and seems silly but really is the right way
> to do this sort of thing. Smaller commits generally are better and I wish
> we broke down a lot of our patch sets more (due to code review being slow I
> think sometimes we tend to bundle too many things into any given patch).

Hello,

+2 on having master branches stable together.  Eventually down the road
we would have a set of (repo, commit sha1) that are known to work
together, we could send that to a git repo and deploy it continuously.


OpenStack does exactly that though it is not used for releasing. But
that gives you a stable set of repo at the tip of their branches.

https://github.com/openstack/openstack#openstack-tracking-repo

Every single commit here had all tests passing.


--
Antoine "hashar" Musso


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Antoine Musso-3
In reply to this post by Brad Jorsch (Anomie)
Le 18/06/2015 16:18, Brad Jorsch (Anomie) a écrit :

> On Thu, Jun 18, 2015 at 9:32 AM, Jon Robson <[hidden email]> wrote:
>> Smaller commits generally are better
>
>
> I'm going to call red herring here. Whether or not smaller commits are
> really better, one patch that does
>
> + if ( detectOldInput( $input ) ) {
> +     $input = upgradeInput( $input );
> +     // wfDeprecated( "You used the old input style!" );
> + }
>
> then a followup that does
>
> -     // wfDeprecated( "You used the old input style!" );
> +     wfDeprecated( "You used the old input style!" );
>
> to be merged 1 second later[1] isn't an example.

Though the second patch should have much more content:
- potentially an announcement (think about MW api.php deprecations)
- migration documentation
- a release note entry

And so on.  So the first patch let devs catch up, the second one is
preparing the proper release/deprecation of the feature.


>  [1]: Assuming Jenkins gets a whole lot faster about merging stuff ;)

Time to get MediaWiki test suite faster :-]  It needs a lot of
refactoring to stop exercising the whole stack for every single "unit"
test :-D


--
Antoine "hashar" Musso


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Overriding Jenkins versus make-work patches

Brad Jorsch (Anomie)
On Thu, Jun 18, 2015 at 10:57 AM, Antoine Musso <[hidden email]> wrote:

> Though the second patch should have much more content:
> - potentially an announcement (think about MW api.php deprecations)
> - migration documentation
> - a release note entry
>

Shouldn't all that be in the *first* patch?

Maybe the second patch could also change "Old-style input is still accepted
for backwards compatibility" to add ", but will raise a deprecation
warning". But that's still a trivial one-line diff.


--
Brad Jorsch (Anomie)
Software Engineer
Wikimedia Foundation
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l