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.
On Wed, Jun 17, 2015 at 10:53 AM, Brad Jorsch (Anomie) bjorsch@wikimedia.org 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:
- 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.
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
#1 sounds right, Jenkins serves us, not vice versa. The core change's commit message should reference the two required extension changes.
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 ;(
On 06/17/2015 09:53 AM, Brad Jorsch (Anomie) wrote:
- 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].
- 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
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 legoktm.wikipedia@gmail.com wrote:
On 06/17/2015 09:53 AM, Brad Jorsch (Anomie) wrote:
- 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].
- 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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
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 < jhernandez@wikimedia.org> wrote:
Option 2 makes the most sense to me:
- Implement new behavior
- Change dependent extensions to use new behavior
- Deprecate old behavior
That said Option 1 seems preferable to 3.
On Wed, Jun 17, 2015 at 10:17 PM, Legoktm legoktm.wikipedia@gmail.com wrote:
On 06/17/2015 09:53 AM, Brad Jorsch (Anomie) wrote:
- 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].
- 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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
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" sbisson@wikimedia.org 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 < jhernandez@wikimedia.org> wrote:
Option 2 makes the most sense to me:
- Implement new behavior
- Change dependent extensions to use new behavior
- Deprecate old behavior
That said Option 1 seems preferable to 3.
On Wed, Jun 17, 2015 at 10:17 PM, Legoktm legoktm.wikipedia@gmail.com wrote:
On 06/17/2015 09:53 AM, Brad Jorsch (Anomie) wrote:
- 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].
- 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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
-- Stephane Bisson Wikimedia Foundation 1 416 270 3830 _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Thu, Jun 18, 2015 at 9:32 AM, Jon Robson jdlrobson@gmail.com 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 ;)
Le 18/06/2015 16:18, Brad Jorsch (Anomie) a écrit :
On Thu, Jun 18, 2015 at 9:32 AM, Jon Robson jdlrobson@gmail.com 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
On Thu, Jun 18, 2015 at 10:57 AM, Antoine Musso hashar+wmf@free.fr 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.
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.
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>
- Merge the core change over Jenkins's objections, then the Flow change
can be merged as normal. But overriding Jenkins sucks.
- 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.
- 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,
wikitech-l@lists.wikimedia.org