Issue 29573947: Noissue - Restructure Build-and-Release-Tools and Sitescripts modules (Closed)
|
Created:
Oct. 12, 2017, 2:45 a.m. by Sebastian Noack Modified:
Oct. 30, 2017, 4:58 p.m. CC:
saroyanm Visibility:
Public. |
DescriptionNoissue - Restructure Build-and-Release-Tools and Sitescripts modules
Patch Set 1 #
Total comments: 11
Patch Set 2 : Named module "Automation" #Patch Set 3 : Added description #
Total comments: 3
MessagesTotal messages: 30
This suggestion is by no means final. But what would you think if we'd move buildtools, codingtools and the development builds part of sitescripts into the same module, with me as module owner and Tristan as module owner peer? In the same step I would remove myself as module owner peer form the Sitescripts module, and Vasily from what was used to be the Build-And-Release-Tools module. That way the modules would match closer with our project teams. And Vasily and me not having an equal say on the the same module could also avoid some philosophical debates in the future. ;) However, of course, for the linters in codingtools, the responsibility of the module owner and his peer is limited to the implementation. But when it comes to what rules to implement everybody will still has an equal voice. https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html File pages/modules.html (left): https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:149: <td><a href="mailto:dave@adblockplus.org">Dave Barker</a></td> Dave, let me know if you would want to be a module owner peer on the "Tooling". https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:166: <a href="https://hg.adblockplus.org/screenshotter">screenshotter</a> I have no idea what this thing was, but apparently it was a legacy Gecko extension, so we can just deprecate it, now. :) https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html File pages/modules.html (right): https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:141: <td>Tooling</td> This name isn't final, but rather a working title. Other options that come to mind would be "Automation" or "Tools". What do you think?
I think this is a good idea overall FWIW. https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html File pages/modules.html (left): https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:149: <td><a href="mailto:dave@adblockplus.org">Dave Barker</a></td> On 2017/10/12 03:20:36, Sebastian Noack wrote: > Dave, let me know if you would want to be a module owner peer on the "Tooling". I don't mind, I think I can still add something but on the other hand I wouldn't be offended if you removed me. I'm hardly doing much Python these days, but on the other hand I do have a good idea about where our tooling could be improved. https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html File pages/modules.html (right): https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:141: <td>Tooling</td> On 2017/10/12 03:20:36, Sebastian Noack wrote: > This name isn't final, but rather a working title. Other options that come to > mind would be "Automation" or "Tools". What do you think? I don't like this name much, but I can't think of anything better right now.
I like the idea of making the module structure closer to the project team structure and I agree with most of the changes in this review. I have two concerns, however: - First one is about `codingtools` repo. I don't mind relinquishing the peership of the extension build tooling, but I'm still interested in working on the developer tools, linters and checkers that live in `codingtools`. Unlike `buildtools`, this repo is not only extension tooling and most development projects depend on it to some extent. Perhaps the module structure should reflect this better. Maybe we could create a separate module for general developer tooling (with you and me as owner/peer in whatever order)? - Second one is about the impact of these changes on our Python developer community. I agree that "philosophical discussions" slow the work down sometimes, but I don't think that they are not useful. Having a common understanding and common ways of working is important and we should try to maintain that. It doesn't have to be through joint module ownership, but that seems to have helped so far. This one is not really a show-stopper, but something to watch out for. Cheers, Vasily https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html File pages/modules.html (right): https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:499: {{module-sitescripts-source <a href="https://hg.adblockplus.org/sitescripts"><fix>sitescripts</fix></a> (except <a href="#tooling"><fix>Tooling</fix></a>)}} Redirecting to Tooling to understand which part of sitescripts is not under this module seems inconvenient for the reader. How about we just say "except sitescripts/extensions/*", same as in Tooling? This would be duplication, of course, but in this case readability seems more important.
To be honest - i wouldn't mind / reject being a module owner peer, but for the time being i guess my experience with especially the sitescripts is too limited, to truely fit that role (at least if can not hope for any expertise from those familiar with it ;) ).
On 2017/10/12 12:34:01, tlucas wrote: > To be honest - i wouldn't mind / reject being a module owner peer, but for the > time being i guess my experience with especially the sitescripts is too limited, > to truely fit that role (at least if can not hope for any expertise from those > familiar with it ;) ). FWIW I think you'd be a good buildtools peer Tristan, even if we didn't restructure the modules.
On 2017/10/12 09:10:48, kzar wrote: > I think this is a good idea overall FWIW. > > https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html > File pages/modules.html (left): > > https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... > pages/modules.html:149: <td><a href="mailto:dave@adblockplus.org">Dave > Barker</a></td> > On 2017/10/12 03:20:36, Sebastian Noack wrote: > > Dave, let me know if you would want to be a module owner peer on the > "Tooling". > > I don't mind, I think I can still add something but on the other hand I wouldn't > be offended if you removed me. I'm hardly doing much Python these days, but on > the other hand I do have a good idea about where our tooling could be improved. > > https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html > File pages/modules.html (right): > > https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... > pages/modules.html:141: <td>Tooling</td> > On 2017/10/12 03:20:36, Sebastian Noack wrote: > > This name isn't final, but rather a working title. Other options that come to > > mind would be "Automation" or "Tools". What do you think? > > I don't like this name much, but I can't think of anything better right now.
On 2017/10/12 12:14:35, Vasily Kuznetsov wrote: > I like the idea of making the module structure closer to the project team > structure and I agree with most of the changes in this review. I have two > concerns, however: > > - First one is about `codingtools` repo. I don't mind relinquishing the peership > of the extension build tooling, but I'm still interested in working on the > developer tools, linters and checkers that live in `codingtools`. Unlike > `buildtools`, this repo is not only extension tooling and most development > projects depend on it to some extent. Perhaps the module structure should > reflect this better. Maybe we could create a separate module for general > developer tooling (with you and me as owner/peer in whatever order)? Note that the responsibility of a module owner and their peers is merely to review the patches and triage issues for that module. Everybody can still contribute. And as I made clear, when it comes to deciding which rules to enforce in flake8-eyeo you will still have the same say you have right now. As you obviously cannot review your own code anyway, not much will change for you. Currently I have to review your contributions to codingtools. After this change, I and/or Tristan would have to review them. The only thing that would change for you is that you are no longer obligated to spend time reviewing changes yourself. Your feedback is still welcome of course, in particular in regard to flake8-eyeo. However, if you don't trust Tristan and me, to fulfill our responsibility, then I guess there is no way around turning codingtools into its own module with both of us on there as owner/peer. But I would rather avoid that overhead. > - Second one is about the impact of these changes on our Python developer > community. I agree that "philosophical discussions" slow the work down > sometimes, but I don't think that they are not useful. Having a common > understanding and common ways of working is important and we should try to > maintain that. It doesn't have to be through joint module ownership, but that > seems to have helped so far. This one is not really a show-stopper, but > something to watch out for. I simply don't have the time to be involved in everything Python anymore. In fact the only situation I still get involved with the Sitescripts module, is when you need to get your code reviewed, which is frustrating at times, when strong concerns I raise get turned down because apparently a decision already has been made in a prior stage or on a higher level. Anyway, I cannot afford anymore to care about what you guys do in sitescripts, and with the CMS in particular. But I rather don't be involved at all, than being dragged into situations, where all decisions already have been made, and I'm forced to formally LGTM code which I cannot in good conscience. Also given that you don't contribute any code to buildtools, and I don't contribute any code to sitescripts, anymore, it is rather questionable whether a module owner peer status is still justified, probably not just for each other inspiration. However, we can always request feedback (on code review or other channels) from people who aren't module owner peer, where it makes sense. On 2017/10/12 13:17:05, kzar wrote: > On 2017/10/12 12:34:01, tlucas wrote: > > To be honest - i wouldn't mind / reject being a module owner peer, but for the > > time being i guess my experience with especially the sitescripts is too > limited, > > to truely fit that role (at least if can not hope for any expertise from those > > familiar with it ;) ). > > FWIW I think you'd be a good buildtools peer Tristan, even if we didn't > restructure the modules. I agree! :) Also the part of sitescripts I suggest to put into the same module as builtools, is merely a wrapper around buildtools (and some REST APIs to upload the builds) that runs as a cronjob in order to automatically publish our developments builds. It already has been established that our team is responsible for this, and within our team, Tristan seems to be the best person to maintain this code. https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html File pages/modules.html (left): https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:149: <td><a href="mailto:dave@adblockplus.org">Dave Barker</a></td> On 2017/10/12 09:10:48, kzar wrote: > On 2017/10/12 03:20:36, Sebastian Noack wrote: > > Dave, let me know if you would want to be a module owner peer on the > "Tooling". > > I don't mind, I think I can still add something but on the other hand I wouldn't > be offended if you removed me. I'm hardly doing much Python these days, but on > the other hand I do have a good idea about where our tooling could be improved. If you want to request improvements, just file issues (regardless whether you are module owner/peer or not). However, if you are module owner peer you have to invest some reviewing patches and triaging issues (not only those you requested yourself). But I leave it up to you. https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html File pages/modules.html (right): https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:499: {{module-sitescripts-source <a href="https://hg.adblockplus.org/sitescripts"><fix>sitescripts</fix></a> (except <a href="#tooling"><fix>Tooling</fix></a>)}} On 2017/10/12 12:14:35, Vasily Kuznetsov wrote: > Redirecting to Tooling to understand which part of sitescripts is not under this > module seems inconvenient for the reader. How about we just say "except > sitescripts/extensions/*", same as in Tooling? This would be duplication, of > course, but in this case readability seems more important. I did it consistently with how Platform and User Interface are doing it. Since there are several paths there, duplicating them inline isn't an option, and I'm not sure whether it is worth to introduce an inconsistency here.
TLDR: LGTM On 2017/10/12 22:28:10, Sebastian Noack wrote: > On 2017/10/12 12:14:35, Vasily Kuznetsov wrote: > > I like the idea of making the module structure closer to the project team > > structure and I agree with most of the changes in this review. I have two > > concerns, however: > > > > - First one is about `codingtools` repo. I don't mind relinquishing the > peership > > of the extension build tooling, but I'm still interested in working on the > > developer tools, linters and checkers that live in `codingtools`. Unlike > > `buildtools`, this repo is not only extension tooling and most development > > projects depend on it to some extent. Perhaps the module structure should > > reflect this better. Maybe we could create a separate module for general > > developer tooling (with you and me as owner/peer in whatever order)? > > Note that the responsibility of a module owner and their peers is merely to > review the patches and triage issues for that module. Everybody can still > contribute. And as I made clear, when it comes to deciding which rules to > enforce in flake8-eyeo you will still have the same say you have right now. > > As you obviously cannot review your own code anyway, not much will change for > you. Currently I have to review your contributions to codingtools. After this > change, I and/or Tristan would have to review them. The only thing that would > change for you is that you are no longer obligated to spend time reviewing > changes yourself. Your feedback is still welcome of course, in particular in > regard to flake8-eyeo. > > However, if you don't trust Tristan and me, to fulfill our responsibility, then > I guess there is no way around turning codingtools into its own module with both > of us on there as owner/peer. But I would rather avoid that overhead. My thinking was that since codingtools is a joint repository which many teams end up using, it would make sense for it to be special and have its own module. And what's the overhead of having a module anyway? Just putting some HTML into the modules page and adding it to Trac? Doesn't sound all too bad. On the other hand, it's probably going to work out fine anyway and if we encounter issues with working this way, we can think about making a separate module later. > > - Second one is about the impact of these changes on our Python developer > > community. I agree that "philosophical discussions" slow the work down > > sometimes, but I don't think that they are not useful. Having a common > > understanding and common ways of working is important and we should try to > > maintain that. It doesn't have to be through joint module ownership, but that > > seems to have helped so far. This one is not really a show-stopper, but > > something to watch out for. > > I simply don't have the time to be involved in everything Python anymore. In > fact the only situation I still get involved with the Sitescripts module, is > when you need to get your code reviewed, which is frustrating at times, when > strong concerns I raise get turned down because apparently a decision already > has been made in a prior stage or on a higher level. > > Anyway, I cannot afford anymore to care about what you guys do in sitescripts, > and with the CMS in particular. But I rather don't be involved at all, than > being dragged into situations, where all decisions already have been made, and > I'm forced to formally LGTM code which I cannot in good conscience. I think the problems we had with sitescripts reviews were related to discussing the substance of the tickets in reviews. And I'm not against discussing tickets, it's just that reviews are not the right place to do it. As for your comments on implementation, I always found them useful and I don't think we had much disagreement there. > Also given that you don't contribute any code to buildtools, and I don't > contribute any code to sitescripts, anymore, it is rather questionable whether a > module owner peer status is still justified, probably not just for each other > inspiration. However, we can always request feedback (on code review or other > channels) from people who aren't module owner peer, where it makes sense. Yeah, you're right and I'm probably overthinking this. https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html File pages/modules.html (right): https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:499: {{module-sitescripts-source <a href="https://hg.adblockplus.org/sitescripts"><fix>sitescripts</fix></a> (except <a href="#tooling"><fix>Tooling</fix></a>)}} On 2017/10/12 22:28:09, Sebastian Noack wrote: > On 2017/10/12 12:14:35, Vasily Kuznetsov wrote: > > Redirecting to Tooling to understand which part of sitescripts is not under > this > > module seems inconvenient for the reader. How about we just say "except > > sitescripts/extensions/*", same as in Tooling? This would be duplication, of > > course, but in this case readability seems more important. > > I did it consistently with how Platform and User Interface are doing it. Since > there are several paths there, duplicating them inline isn't an option, and I'm > not sure whether it is worth to introduce an inconsistency here. Acknowledged.
LGTM
https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html File pages/modules.html (right): https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:141: <td>Tooling</td> On 2017/10/12 09:10:48, kzar wrote: > On 2017/10/12 03:20:36, Sebastian Noack wrote: > > This name isn't final, but rather a working title. Other options that come to > > mind would be "Automation" or "Tools". What do you think? > > I don't like this name much, but I can't think of anything better right now. FWIW, i personally prefer the "Automation" suggestion.
On 2017/10/13 16:35:42, tlucas wrote: > https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html > File pages/modules.html (right): > > https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... > pages/modules.html:141: <td>Tooling</td> > On 2017/10/12 09:10:48, kzar wrote: > > On 2017/10/12 03:20:36, Sebastian Noack wrote: > > > This name isn't final, but rather a working title. Other options that come > to > > > mind would be "Automation" or "Tools". What do you think? > > > > I don't like this name much, but I can't think of anything better right now. > > FWIW, i personally prefer the "Automation" suggestion. I guess since you and Sebastian will be in charge of the module the naming is up to you guys.
Nice, it seems we are all set. I promoted Felix to reviewer, to make him give his final approval, since he is the "content owner" of the /modules page. Added Websites module owner and peer to CC, to review the HTML changes (if they wish too). https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html File pages/modules.html (right): https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... pages/modules.html:141: <td>Tooling</td> On 2017/10/13 16:35:42, tlucas wrote: > On 2017/10/12 09:10:48, kzar wrote: > > On 2017/10/12 03:20:36, Sebastian Noack wrote: > > > This name isn't final, but rather a working title. Other options that come > to > > > mind would be "Automation" or "Tools". What do you think? > > > > I don't like this name much, but I can't think of anything better right now. > > FWIW, i personally prefer the "Automation" suggestion. Done.
On 2017/10/13 20:01:56, Sebastian Noack wrote: > Nice, it seems we are all set. > > I promoted Felix to reviewer, to make him give his final approval, since he is > the "content owner" of the /modules page. > > Added Websites module owner and peer to CC, to review the HTML changes (if they > wish too). > > https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html > File pages/modules.html (right): > > https://codereview.adblockplus.org/29573947/diff/29573948/pages/modules.html#... > pages/modules.html:141: <td>Tooling</td> > On 2017/10/13 16:35:42, tlucas wrote: > > On 2017/10/12 09:10:48, kzar wrote: > > > On 2017/10/12 03:20:36, Sebastian Noack wrote: > > > > This name isn't final, but rather a working title. Other options that come > > to > > > > mind would be "Automation" or "Tools". What do you think? > > > > > > I don't like this name much, but I can't think of anything better right now. > > > > FWIW, i personally prefer the "Automation" suggestion. > > Done. If we rename the module here, should the Trac component also be renamed? Seems like it would be less confusing that way.
On 2017/10/14 11:34:47, Vasily Kuznetsov wrote: > If we rename the module here, should the Trac component also be renamed? Seems > like it would be less confusing that way. Of course, we have to rename the component on Trac as well.
I see n distinct changes here: 1. Rename the "Build and Release Tools" module to "Automation" (I'll call it "BART" from now on, for brevity's sake). 2. Remove Dave and Vasily as peers from BART. 3. Add Tristan as peer to BART. 4. Remove screenshotter from BART. 5. Move sitescripts/extensions/* from Sitescripts to BART. 6. Move codingtools from Sitescripts to BART. 7. Remove Sebastian as peer from Sitescripts. That's quite a few, so I'll first eliminate those that I find uncontroversial: 3, 4, 5 and 7. Feel free to make these changes right away in a separate commit. That leaves 1, 2 and 6 to be discussed. I can only say whether or not we should do these things once we've answered another question: What is the BART module going to be? Is it going to be: A. A central tooling/automation repository used across all (or most) our projects. B. Tooling/automation mostly specific to ABP WebExt. If we go for A, my take is: 1 = Yes, 2 = Vasily should be owner or peer, 6 = Yes. If we go for B, my take is: 1 = Should be less generic, 2 = Yes, 6 = No - Sitescripts isn't a good place either, so it'd be best to make it a separate module. B is in my view the status quo. The one thing I can think of that suggests otherwise is ensure_dependencies.py, which is used in most projects. But I don't see that as a problem: If projects want to use it, they can. If they have use cases not supported by it, it's their problem. If B is the status quo, the question becomes: Do we want to develop BART into A? Based on how I see the situation, I prefer sticking to B. Other projects can still use code from BART as far as it's useful to them, but if they have needs not met by it, they should just address those themselves (which can be by contributing back, but only if BART is interested in the functionality). But I'm curious to hear what others think.
Do we intend to translate this page in the future?
On 2017/10/17 11:26:57, juliandoucette wrote: > Do we intend to translate this page in the future? I don't think so. But let's discuss that later/separately, we have enough to discuss here as it is.
On 2017/10/17 11:23:36, Felix Dahlke wrote: > ... But I'm curious to hear what others think. Well FWIW I pretty much agree with Sebastian's original suggestion, I don't like how the build and development tooling is conflated with the code that runs on our servers. Like Sebastian pointed out the change would make the modules match much closer to our project teams, and IMO that would make things generally go more smoothly. I agree that the module could be named better, but I don't have any suggestions there. I see your point about Buildtools being generally for WebExt builds, which moving codingtools in would contradict. But IMO it makes more sense than it living in Sitescripts.
On 2017/10/17 11:23:36, Felix Dahlke wrote: > I see n distinct changes here: > > 1. Rename the "Build and Release Tools" module to "Automation" (I'll call it > "BART" from now on, for brevity's sake). > 2. Remove Dave and Vasily as peers from BART. > 3. Add Tristan as peer to BART. > 4. Remove screenshotter from BART. > 5. Move sitescripts/extensions/* from Sitescripts to BART. > 6. Move codingtools from Sitescripts to BART. > 7. Remove Sebastian as peer from Sitescripts. > > That's quite a few, so I'll first eliminate those that I find uncontroversial: > 3, 4, 5 and 7. Feel free to make these changes right away in a separate commit. > > That leaves 1, 2 and 6 to be discussed. I can only say whether or not we should > do these things once we've answered another question: What is the BART module > going to be? Is it going to be: > > A. A central tooling/automation repository used across all (or most) our > projects. > B. Tooling/automation mostly specific to ABP WebExt. > > If we go for A, my take is: 1 = Yes, 2 = Vasily should be owner or peer, 6 = > Yes. > > If we go for B, my take is: 1 = Should be less generic, 2 = Yes, 6 = No - > Sitescripts isn't a good place either, so it'd be best to make it a separate > module. > > B is in my view the status quo. The one thing I can think of that suggests > otherwise is ensure_dependencies.py, which is used in most projects. But I don't > see that as a problem: If projects want to use it, they can. If they have use > cases not supported by it, it's their problem. > > If B is the status quo, the question becomes: Do we want to develop BART into A? > > Based on how I see the situation, I prefer sticking to B. Other projects can > still use code from BART as far as it's useful to them, but if they have needs > not met by it, they should just address those themselves (which can be by > contributing back, but only if BART is interested in the functionality). But I'm > curious to hear what others think. Good analysis! I also prefer B, primarily because it's more in line with the project structure. I also agree with points B1 and B6.
I'm not sure how much more generic the module would be if we go ahead with the plan everybody previously agreed on. First of all adblockplus.org/modules is specific to Adblock Plus. So an "Automation" module in this context would obviously not be responsible for Flattr's or general eyeo automation. Then again we already have logic in sitescripts/extensions/* covering IE and Android, which aren't part of the ABP WebExt team either, not to mention ensure_depedencies.py in buildtools. But it's probably not necessary (or even possible), to have all modules match the project team structure to 100%. In the end it seems all to be again about where codingtools should go. But as everybody agrees that it doesn't belong into Sitescripts either, I don't see why it would be such a big deal to move it to the Build-and-Release-Tools/Automation module. In particular, since as Dave explained well, it is certainly more out of place among the code that runs on our servers, than it would be in our build tooling. So it seems the whole controversy is just about not having Vasily as module owner/peer for codingtools. However, as I explained before, everybody can contribute, not only the module owner/peers. Then again, Vasily doesn't even seem to be an active contributor to codingtools (4 commits, all only touching patchconv) vs Wladimir (10 commits), Dave (14 commits) and myself (15 commits). Besides past commitment, another thing to be considered are resources to fulfill the responsibilities of a module owner/peer from now on, where I rather see Tristan than Vasily. So even if codingtools would become a separate module, having Vasily as module owner wouldn't be the most obvious choice.
On 2017/10/17 20:52:16, Sebastian Noack wrote: > I'm not sure how much more generic the module would be if we go ahead with > the plan everybody previously agreed on. First of all adblockplus.org/modules is > specific to Adblock Plus. So an "Automation" module in this context would > obviously not be responsible for Flattr's or general eyeo automation. > Then again we already have logic in sitescripts/extensions/* covering IE > and Android, which aren't part of the ABP WebExt team either, not to mention > ensure_depedencies.py in buildtools. But it's probably not necessary (or even > possible), to have all modules match the project team structure to 100%. Like I said, I don't mind seeing ensure_dependencies.py as an exception that proves the rule. The same goes for ABP for Android and IE's update manifest generation logic. We could either move this (back) to Sitescripts, or consider these two legacy and don't worry about it, the latter is fine by me. These things aside, the BART module (I'll stick to my nick name for "The Module Formerly Known As Build and Release Tools") is specific to Chrome/Firefox/Safari/Edge/Opera, all of which's code (XUL-based ABP aside) reside in the Platform module. Then there's a few component modules that don't need build/release automation in the same sense: Core and UI. And then there's lots of other things that don't really use anything other than ensure_dependencies.py from BART: Libadblockplus, Libadblockplus-android, ABB for Android, ABP for SBrowser, ABP for iOS, ABB for iOS (soon to be a module), Infrastructure, Sitescripts, Websites. (The latter three are arguably more "eyeo" things than "ABP" things.) It doesn't sound as if anyone wants to change that, which means we're going for B: We won't change the status quo here. As far as the name is concerned, I do find "Automation" too generic. But one could argue that the exact same is true for "UI" - it's fairly specific to Platform as well. So I'm fine with changing the name to Automation, IF we add a description that explains where/how it's actually used. > In the end it seems all to be again about where codingtools should go. But as > everybody agrees that it doesn't belong into Sitescripts either, I don't see why > it would be such a big deal to move it to the Build-and-Release-Tools/Automation > module. In particular, since as Dave explained well, it is certainly more out of > place among the code that runs on our servers, than it would be in our build > tooling. So it seems the whole controversy is just about not having Vasily as > module owner/peer for codingtools. > > However, as I explained before, everybody can contribute, not only the module > owner/peers. Then again, Vasily doesn't even seem to be an active contributor to > codingtools (4 commits, all only touching patchconv) vs Wladimir (10 commits), > Dave (14 commits) and myself (15 commits). Besides past commitment, another > thing to be considered are resources to fulfill the responsibilities of a module > owner/peer from now on, where I rather see Tristan than Vasily. > So even if codingtools would become a separate module, having Vasily as module > owner wouldn't be the most obvious choice. Alright, codingtools. I said before that I don't see it belonging to either BART, nor Sitescripts, it's mostly its own thing. However, I'm fine with keeping it in an unrelated module for now and worrying about whether it should be its own module later, when it's used for more things. Sebastian and I had a quick IRC conversation about this, module ownership is slightly different with codingtools. We have three types of things in there: 1. Useful scripts: hgreview.py, patchconv 2. Useful configurations: editorconfig, hgrc 3. Official linter configurations: eslint-config-eyeo, flake8-abp, flake8-eyeo (soon to land: Java, C++ and iOS specific stuff) There's nobody at the company who should/can review all of these. It depends on the individual case. It's a bit like our websites, where we have the orthogonal implicit concept of "content ownership" in addition to module ownership. So I see the module owner's duties here as: a. Ensuring we don't make a mess in the repo, with the directory structure etc. b. Ensuring we don't add things that are unnecessarily big, or would make more sense as their own dedicated repo. c. Reviewing/maintaining scripts that are (at least somewhat) official, like hgreview.py. Although it can also make sense to defer that responsibility to someone else for some scripts. I don't care who handles this, as long as it's handled sensibly. So if Sebastian, Vasily and Tristan are fine with moving it to BART, so am I.
I'm off tomorrow and next week and I don't want to block this, so I've moved myself to CC - assuming my remarks will get addressed.
Sounds good to me. In other words, as I said before, the rules enforced by our linters (and other official configurations in codingtools) is out of scope of the responsibility of the module owner/peers. Everybody seems to agree on this already. So the only question is how to document this. As far as I'm concerned, having this clarified and agreed on, here, in the code review is sufficient. After all, it's not that different from other modules, e.g. the User Interface module owner usually doesn't decide on the design, and the Websites module owner not on the content, but rather on how these are implemented.
On 2017/10/19 19:06:58, Sebastian Noack wrote: > Sounds good to me. In other words, as I said before, the rules enforced by our > linters (and other official configurations in codingtools) is out of scope of > the responsibility of the module owner/peers. Everybody seems to agree on this > already. So the only question is how to document this. As far as I'm concerned, > having this clarified and agreed on, here, in the code review is sufficient. > After all, it's not that different from other modules, e.g. the User Interface > module owner usually doesn't decide on the design, and the Websites module owner > not on the content, but rather on how these are implemented. IF i managed to keep up with every detail - and since we are obviously sticking to B - the current situation looks like this: Good to go: > 2. Remove Dave and Vasily as peers from BART. > 3. Add Tristan as peer to BART. > 4. Remove screenshotter from BART. > 5. Move sitescripts/extensions/* from Sitescripts to BART. > 7. Remove Sebastian as peer from Sitescripts. Whereas the following needs to be clarified: > 1. Rename the "Build and Release Tools" module to "Automation" (I'll call it > "BART" from now on, for brevity's sake). -> Either add a description and/or name it less generic (could IMHO lead us back to "Build and Release Tools" - Maybe something like "Deployment Basics"?) > 6. Move codingtools from Sitescripts to BART. -> NOT an option right now, since we see it more as a future independent module with both "module" and "content ownership". That again leaves us with the following TODOs for this very review: a) Revert the move of "codingtools" and address it in a different issue / hub ticket / review. b) Take the hurdle and think of a more descriptive name or add a description (and adapt everywhere else in the changes). Please point me at things i forgot - however, if the above is correct, i'm ok with everything discussed so far. Cheers!
Well, as per Felix comment above (and the discussion I had with him on IRC), he is fine with: * Naming the module "Automation" * Having "codingtools" in there Before Felix jumped in, this was what everybody else already agreed on. So I don't think that this needs any further discussion, now. But just in case, I personally don't care much about the name, though I think "Automation" is the best we came up with so far. However, I would have a problem no longer being module owner for codingtools. Also everybody agrees that it is at least less out of place in "Automation" than it is in "Sitescripts", and if in the end the same person will be module owner anyway, there is not much of a point to make it a separate module. Otherwise, we could easily split "Sitescripts" into 10 modules with the same owner. This at least was the conclusion that Felix and I had when we discussed this on IRC. The only thing Felix is asking for, now, is to add a description to the "Automation" module.
Added the description. https://codereview.adblockplus.org/29573947/diff/29583642/pages/modules.html File pages/modules.html (right): https://codereview.adblockplus.org/29573947/diff/29583642/pages/modules.html#... pages/modules.html:68: <td>{{module-ui-description The User Interface of the Adblock Plus browser extension.}}</td> While on it, I also fixed this description, since adblockpluschrome is the only implementation of the user interface provided by this module, now.
LGTM https://codereview.adblockplus.org/29573947/diff/29583642/pages/modules.html File pages/modules.html (right): https://codereview.adblockplus.org/29573947/diff/29583642/pages/modules.html#... pages/modules.html:68: <td>{{module-ui-description The User Interface of the Adblock Plus browser extension.}}</td> On 2017/10/20 01:53:38, Sebastian Noack wrote: > While on it, I also fixed this description, since adblockpluschrome is the only > implementation of the user interface provided by this module, now. Fair enough, but Thomas should check this change too. I've added him.
LGTM https://codereview.adblockplus.org/29573947/diff/29583642/pages/modules.html File pages/modules.html (right): https://codereview.adblockplus.org/29573947/diff/29583642/pages/modules.html#... pages/modules.html:68: <td>{{module-ui-description The User Interface of the Adblock Plus browser extension.}}</td> On 2017/10/20 11:48:13, kzar wrote: > On 2017/10/20 01:53:38, Sebastian Noack wrote: > > While on it, I also fixed this description, since adblockpluschrome is the > only > > implementation of the user interface provided by this module, now. > > Fair enough, but Thomas should check this change too. I've added him. I'm ok with this change.
HTML change LGTM. I'll stay out of the other discussions.
Just in case this is waiting for everyone's approval : LGTM 
|
