Issue 29325436: Issue 1281 - Introduce module buildbot (Closed)
|
Created:
Sept. 1, 2015, 1:30 p.m. by mathias Modified:
Feb. 20, 2016, 1:25 p.m. Reviewers:
Felix Dahlke CC:
Fred Visibility:
Public. |
DescriptionIssue 1281 - Introduce module buildbot
Patch Set 1 #Patch Set 2 : Issue 1281 - Rebase branch buildbot on top of upstream #
Total comments: 40
Patch Set 3 : Issue 1281 - Address review feedback #
Total comments: 26
Patch Set 4 : Issue 1281 - Fix documentation, improve configuration parameter handling #
Total comments: 3
MessagesTotal messages: 20
Just looked at the first few files so far. I won't be able to look at the rest today, so here go those comments already. https://codereview.adblockplus.org/29325436/diff/29326009/hiera/roles/buildse... File hiera/roles/buildserver.yaml (right): https://codereview.adblockplus.org/29325436/diff/29326009/hiera/roles/buildse... hiera/roles/buildserver.yaml:10: castus: I would vote for more descriptive names in line with our usual naming scheme, e.g. "buildslave1" or maybe even "linuxbuildslave1". What do you think? https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... File modules/adblockplus/manifests/buildserver.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... modules/adblockplus/manifests/buildserver.pp:35: class adblockplus::buildserver ( Why is the module called "adblockplus" and not "buildserver"? https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... modules/adblockplus/manifests/buildserver.pp:35: class adblockplus::buildserver ( I'm wondering if we should call the module "buildmaster" instead of "buildserver", and the servers "buildmaster1" etc. instead of "build1". No strong opinion, just wondering if that'd make things more clear. What do you think? https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... modules/adblockplus/manifests/buildserver.pp:38: $project = {}, This one isn't documented above. https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... modules/adblockplus/manifests/buildserver.pp:49: master_service => hiera('buildbot::master_service', 'running'), Wrong indentation?
(I will update the acknowledged parts accordingly after your first review has finished.) https://codereview.adblockplus.org/29325436/diff/29326009/hiera/roles/buildse... File hiera/roles/buildserver.yaml (right): https://codereview.adblockplus.org/29325436/diff/29326009/hiera/roles/buildse... hiera/roles/buildserver.yaml:10: castus: On 2015/09/29 14:04:21, Felix Dahlke wrote: > I would vote for more descriptive names in line with our usual naming scheme, > e.g. "buildslave1" or maybe even "linuxbuildslave1". What do you think? Sure, this is actually just meant as an example. Note that an adblockplus::buildserver::slaves key in the YAML files in module private will overwrite this one, but not become merged. And until we actually have some useful examples implemented I somehow struggle with finding more descriptive names. Maybe "foo" or "bar"? https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... File modules/adblockplus/manifests/buildserver.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... modules/adblockplus/manifests/buildserver.pp:35: class adblockplus::buildserver ( On 2015/09/29 14:04:22, Felix Dahlke wrote: > I'm wondering if we should call the module "buildmaster" instead of > "buildserver", and the servers "buildmaster1" etc. instead of "build1". No > strong opinion, just wondering if that'd make things more clear. What do you > think? Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... modules/adblockplus/manifests/buildserver.pp:35: class adblockplus::buildserver ( On 2015/09/29 14:04:22, Felix Dahlke wrote: > Why is the module called "adblockplus" and not "buildserver"? The first part (adblockplus::) is considered a namespace for setups that are specific for the Adblock Plus project. The buildbot module introduced and used here, in contrast, does not assume any implementation details in that direction, while adbockplus::buildserver (resp. adblockplus::buildmaster) reflects a particular use case. The intention is a) a clean separation and b) a directory structure easy to follow and document. Note that this approach has been chosen in other recent implementations as well (e.g. ELK), thus we may need to discuss this in another context if you have any objections. https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... modules/adblockplus/manifests/buildserver.pp:38: $project = {}, On 2015/09/29 14:04:22, Felix Dahlke wrote: > This one isn't documented above. Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... modules/adblockplus/manifests/buildserver.pp:49: master_service => hiera('buildbot::master_service', 'running'), On 2015/09/29 14:04:21, Felix Dahlke wrote: > Wrong indentation? Acknowledged.
Here go some more comments. Now I only need to look at master.pp, slave.pp and master.cfg.erb. https://codereview.adblockplus.org/29325436/diff/29326009/hiera/roles/buildse... File hiera/roles/buildserver.yaml (right): https://codereview.adblockplus.org/29325436/diff/29326009/hiera/roles/buildse... hiera/roles/buildserver.yaml:10: castus: On 2015/09/29 14:18:56, mathias wrote: > On 2015/09/29 14:04:21, Felix Dahlke wrote: > > I would vote for more descriptive names in line with our usual naming scheme, > > e.g. "buildslave1" or maybe even "linuxbuildslave1". What do you think? > > Sure, this is actually just meant as an example. Note that an > adblockplus::buildserver::slaves key in the YAML files in module private will > overwrite this one, but not become merged. And until we actually have some > useful examples implemented I somehow struggle with finding more descriptive > names. Maybe "foo" or "bar"? Can't we leave those out then? As you see, it confused me :D https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... File modules/adblockplus/manifests/buildserver.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... modules/adblockplus/manifests/buildserver.pp:35: class adblockplus::buildserver ( On 2015/09/29 14:18:56, mathias wrote: > On 2015/09/29 14:04:22, Felix Dahlke wrote: > > Why is the module called "adblockplus" and not "buildserver"? > > The first part (adblockplus::) is considered a namespace for setups that are > specific for the Adblock Plus project. The buildbot module introduced and used > here, in contrast, does not assume any implementation details in that direction, > while adbockplus::buildserver (resp. adblockplus::buildmaster) reflects a > particular use case. > > The intention is a) a clean separation and b) a directory structure easy to > follow and document. Note that this approach has been chosen in other recent > implementations as well (e.g. ELK), thus we may need to discuss this in another > context if you have any objections. That's news to me, but fine by me - as long as we do this consistently with all our "server modules". https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/init.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:27: # The anchestor of the default buildbot::master $basedir. Typo. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:29: # [*master_pacakges*] Typo. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:36: # The status 'buildmaster' service status to ensure, if any. One "status" should suffice. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:65: class buildbot ( For some reason, I haven't yet found the part where buildbot is actually being installed. But assuming there are separate packages for master and slave - why configure them together like this? In practice, a node should be either a master or a slave, not both. If it's one package for both, and it's being installed here somehow, nevermind.
OK, now I'm finished. Did a rather high level review, but assuming it works, looks good all in all. So did I understand that correctly that, for now, we will add build configurations to the adblockplus::buildmaster module, using buildbot::master::fragment? We talked about more sophisticated ways of doing this in a dedicated repository a while ago, but I'm happy with anything that just gets us going at this point. We can always improve it later. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/master.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:8: # implicitly, for example) or use a custom aproach for setting up the Typo. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:15: # if it's anchestors are present, relied upon the builtin defaults. Typo. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:26: # [*http_port*] Nit: Maybe call this one "web_port" or something, and change the description of the option a bit, to emphasize that this is the port for the web interface? https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:30: # [*port*] Maybe we should call this "slave_port" or something that emphasizes what this port is used for? https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:41: # [*slots*] Frankly, I didn't understand what this is for, with the name "slots"? I don't have a great idea for something better though - "slave_credentials" maybe? https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/slave.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/slave.pp:9: # if it's anchestors are present, relied upon the builtin defaults. Typo.
On 2015/10/02 12:39:43, Felix Dahlke wrote: > So did I understand that correctly that, for now, we will add build > configurations to the adblockplus::buildmaster module, using > buildbot::master::fragment? We talked about more sophisticated ways of doing > this in a dedicated repository a while ago, but I'm happy with anything that > just gets us going at this point. We can always improve it later. Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/hiera/roles/buildse... File hiera/roles/buildserver.yaml (right): https://codereview.adblockplus.org/29325436/diff/29326009/hiera/roles/buildse... hiera/roles/buildserver.yaml:10: castus: On 2015/10/02 12:16:26, Felix Dahlke wrote: > Can't we leave those out then? As you see, it confused me :D Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... File modules/adblockplus/manifests/buildserver.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/adblockplus... modules/adblockplus/manifests/buildserver.pp:35: class adblockplus::buildserver ( On 2015/10/02 12:16:26, Felix Dahlke wrote: > On 2015/09/29 14:18:56, mathias wrote: > > On 2015/09/29 14:04:22, Felix Dahlke wrote: > > > Why is the module called "adblockplus" and not "buildserver"? > > > > The first part (adblockplus::) is considered a namespace for setups that are > > specific for the Adblock Plus project. The buildbot module introduced and used > > here, in contrast, does not assume any implementation details in that > direction, > > while adbockplus::buildserver (resp. adblockplus::buildmaster) reflects a > > particular use case. > > > > The intention is a) a clean separation and b) a directory structure easy to > > follow and document. Note that this approach has been chosen in other recent > > implementations as well (e.g. ELK), thus we may need to discuss this in > another > > context if you have any objections. > > That's news to me, but fine by me - as long as we do this consistently with all > our "server modules". Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/init.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:27: # The anchestor of the default buildbot::master $basedir. On 2015/10/02 12:16:26, Felix Dahlke wrote: > Typo. Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:29: # [*master_pacakges*] On 2015/10/02 12:16:26, Felix Dahlke wrote: > Typo. Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:36: # The status 'buildmaster' service status to ensure, if any. On 2015/10/02 12:16:26, Felix Dahlke wrote: > One "status" should suffice. Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:65: class buildbot ( On 2015/10/02 12:16:26, Felix Dahlke wrote: > For some reason, I haven't yet found the part where buildbot is actually being > installed. Types buildbot::master and buildbot::slave will ensure_packages($::buildbot::{master,slave}_packages) when necessary. In addition, when either ${master,slave}_service is defined, ensure_packages(${master,slave}_packages) is invoked by class buildbot (see the bottom of this file). > But assuming there are separate packages for master and slave - why > configure them together like this? Note that all resources contained by class buildbot are virtual. Entities within the buildbot namespace realize those when necessary. The options here do not only configure these virtual resources, but are also re-used by the other entities. I prefer this method over the more common approach where one would have an almost empty buildbot class, because.. 1. If using internal variables (e.g. to inherit from params, and then use $var directly), it's less transparent, confusing authors about where what has been defined, and often feature a misleading name ($foo::params). 2. If using actual parameters (e.g. $foo::params::var), it's quite repetetive without any gain over the inclusion here whatsoever. 3. Package specific decisions, ordering and quirks would be scattered over two different files, at least. In cnotrast, all resources in class buildbot address package-specific aspects, not the actual buildbot master or slave configuration, which are done in types ::master and ::slave. 4. Using the root class instead of params avoids common Puppet bugs with inclusion order, e.g. https://projects.puppetlabs.com/issues/11875 (that particular one was the reason for multiple issues in our tracker before, for both our modules and 3rd party ones), whilst being agnostic to the Puppet version in use (2 or 3). https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/master.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:8: # implicitly, for example) or use a custom aproach for setting up the On 2015/10/02 12:39:42, Felix Dahlke wrote: > Typo. Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:15: # if it's anchestors are present, relied upon the builtin defaults. On 2015/10/02 12:39:43, Felix Dahlke wrote: > Typo. Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:26: # [*http_port*] On 2015/10/02 12:39:42, Felix Dahlke wrote: > Nit: Maybe call this one "web_port" or something, and change the description of > the option a bit, to emphasize that this is the port for the web interface? Frankly I believe the http_* prefix to be better. It's more precise than web_*, highlighting that it's not an HTTPS port, for example. But I agree that the documentation should be more verbose regarding the purpose of this option. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:30: # [*port*] On 2015/10/02 12:39:43, Felix Dahlke wrote: > Maybe we should call this "slave_port" or something that emphasizes what this > port is used for? Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:41: # [*slots*] On 2015/10/02 12:39:42, Felix Dahlke wrote: > Frankly, I didn't understand what this is for, with the name "slots"? I don't > have a great idea for something better though - "slave_credentials" maybe? Acknowledged. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/slave.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/slave.pp:9: # if it's anchestors are present, relied upon the builtin defaults. On 2015/10/02 12:39:43, Felix Dahlke wrote: > Typo. Acknowledged.
https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/init.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:65: class buildbot ( On 2015/10/02 13:47:40, mathias wrote: > On 2015/10/02 12:16:26, Felix Dahlke wrote: > > For some reason, I haven't yet found the part where buildbot is actually being > > installed. > > Types buildbot::master and buildbot::slave will > ensure_packages($::buildbot::{master,slave}_packages) when necessary. In > addition, when either ${master,slave}_service is defined, > ensure_packages(${master,slave}_packages) is invoked by class buildbot (see the > bottom of this file). > > > But assuming there are separate packages for master and slave - why > > configure them together like this? > > Note that all resources contained by class buildbot are virtual. Entities within > the buildbot namespace realize those when necessary. The options here do not > only configure these virtual resources, but are also re-used by the other > entities. > > I prefer this method over the more common approach where one would have an > almost empty buildbot class, because.. > > 1. If using internal variables (e.g. to inherit from params, and then use $var > directly), it's less transparent, confusing authors about where what has been > defined, and often feature a misleading name ($foo::params). > > 2. If using actual parameters (e.g. $foo::params::var), it's quite repetetive > without any gain over the inclusion here whatsoever. > > 3. Package specific decisions, ordering and quirks would be scattered over two > different files, at least. In cnotrast, all resources in class buildbot address > package-specific aspects, not the actual buildbot master or slave configuration, > which are done in types ::master and ::slave. > > 4. Using the root class instead of params avoids common Puppet bugs with > inclusion order, e.g. https://projects.puppetlabs.com/issues/11875 (that > particular one was the reason for multiple issues in our tracker before, for > both our modules and 3rd party ones), whilst being agnostic to the Puppet > version in use (2 or 3). Oh, now I get it, the packages to use and some details about them are configurable... To be honest, I'm not happy with the complexity this adds. It makes things harder to understand (at least for me), it's inconsistent with how we do things in the other modules, and I don't actually see a use case for it. If I was using this class, I would have used it wrong and spent some time debugging. To me, $slave_packages seemed to be about packages I need in order to compile my stuff, such as `build-essentials`, so I would have overwritten this. I don't fully understand what problems this approach solves, but since consistency is most important, we should do it the same way we do it in our other modules here, and continue this discussion elsewhere. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/master.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:26: # [*http_port*] On 2015/10/02 13:47:40, mathias wrote: > On 2015/10/02 12:39:42, Felix Dahlke wrote: > > Nit: Maybe call this one "web_port" or something, and change the description > of > > the option a bit, to emphasize that this is the port for the web interface? > > Frankly I believe the http_* prefix to be better. It's more precise than web_*, > highlighting that it's not an HTTPS port, for example. But I agree that the > documentation should be more verbose regarding the purpose of this option. The problem with "http" is that it is a protocol - it doesn't say at all what happens on this port, just what protocol is being used. Could be an API endpoint, or whatever. But I admit that "web" is not _much_ more telling, and I'm fine with leaving it as it is, but would prefer a comment that makes it clear that this is for the web interface.
https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/init.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:65: class buildbot ( > Oh, now I get it, the packages to use and some details about them are > configurable... Providing those options is a trait of this approach, not the reason why it has been chosen. > To be honest, I'm not happy with the complexity this adds. It > makes things harder to understand (at least for me), it's inconsistent with how > we do things in the other modules, and I don't actually see a use case for it. Actually it reduces complexity, but I agree that this is not as trivial to understand as the more common approach mentioned before. > If I was using this class, I would have used it wrong and spent some time > debugging. To me, $slave_packages seemed to be about packages I need in order to > compile my stuff, such as `build-essentials`, so I would have overwritten this. The documentation of types buildbot::master and buidlbot::slave is quite verbose in my opinion. So is that really an issue, or maybe attributed to splitting up the review in multiple sessions? Maybe it would be enough to extend the documentation of class buildbot by actual examples including the other types; one who would "just use" the module in the future may understand it better then. > I don't fully understand what problems this approach solves, but since > consistency is most important, we should do it the same way we do it in our > other modules here, and continue this discussion elsewhere. While consistency is of course an important aspect, there is also reason to evolve in "how we do things", especially when and where more sophisticated solutions are required (the Puppet bug mentioned before, for example). The approach here could serve as an example for future modules and, if proven to actually be better, become the new "how we do things". Note also that most of the existing modules neither comply with the officially chosen coding and style guides, nor employ any advanced techniques in general. Anyway, I do read your notion of this being not quite optimal yet. Let's see if I can combine both sides concerns into a single approach, one that should also ease insight in the concept of the applied pattern. https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/master.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/master.pp:26: # [*http_port*] On 2015/10/02 15:44:27, Felix Dahlke wrote: > On 2015/10/02 13:47:40, mathias wrote: > > On 2015/10/02 12:39:42, Felix Dahlke wrote: > > > Nit: Maybe call this one "web_port" or something, and change the description > > of > > > the option a bit, to emphasize that this is the port for the web interface? > > > > Frankly I believe the http_* prefix to be better. It's more precise than > web_*, > > highlighting that it's not an HTTPS port, for example. But I agree that the > > documentation should be more verbose regarding the purpose of this option. > > The problem with "http" is that it is a protocol - it doesn't say at all what > happens on this port, just what protocol is being used. Could be an API > endpoint, or whatever. > > But I admit that "web" is not _much_ more telling, and I'm fine with leaving it > as it is, but would prefer a comment that makes it clear that this is for the > web interface. Acknowledged.
https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/init.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:65: class buildbot ( On 2015/10/02 16:25:53, mathias wrote: > > Oh, now I get it, the packages to use and some details about them are > > configurable... > > Providing those options is a trait of this approach, not the reason why it has > been chosen. > > > To be honest, I'm not happy with the complexity this adds. It > > makes things harder to understand (at least for me), it's inconsistent with > how > > we do things in the other modules, and I don't actually see a use case for it. > > Actually it reduces complexity, but I agree that this is not as trivial to > understand as the more common approach mentioned before. > > > If I was using this class, I would have used it wrong and spent some time > > debugging. To me, $slave_packages seemed to be about packages I need in order > to > > compile my stuff, such as `build-essentials`, so I would have overwritten > this. > > The documentation of types buildbot::master and buidlbot::slave is quite verbose > in my opinion. So is that really an issue, or maybe attributed to splitting up > the review in multiple sessions? Maybe it would be enough to extend the > documentation of class buildbot by actual examples including the other types; > one who would "just use" the module in the future may understand it better then. > > > I don't fully understand what problems this approach solves, but since > > consistency is most important, we should do it the same way we do it in our > > other modules here, and continue this discussion elsewhere. > > While consistency is of course an important aspect, there is also reason to > evolve in "how we do things", especially when and where more sophisticated > solutions are required (the Puppet bug mentioned before, for example). The > approach here could serve as an example for future modules and, if proven to > actually be better, become the new "how we do things". > > Note also that most of the existing modules neither comply with the officially > chosen coding and style guides, nor employ any advanced techniques in general. > > Anyway, I do read your notion of this being not quite optimal yet. Let's see if > I can combine both sides concerns into a single approach, one that should also > ease insight in the concept of the applied pattern. > Please note that I'm fine with what the buildbot class here does, how I see it it reduces duplication. I'm solely questioning why $*_config etc. are parameters, rather than variables. If there is no other good way to share these constants between buildbot, buildbot::master and buildbot::slave, I guess it's fine... Surely, improving how we do things is important. But I'd prefer to do that by first agreeing on the approach, ensuring it is reflected in our coding style, if necessary, and having a plan for enforcing it to the old code over time.
Issue 1281 - Migrate unused parameters to class variables
Issue 1281 - Rename $buildbot::master::{slots => slave_credentials} parameter
Issue 1281 - Rename $buildbot::master::{slave => slave_port} parameter
Issue 1281 - Fix buildboti::slave documentation typos
Issue 1281 - Fix buildbot::master documentation typos
Issue 1281 - Fix buildbot documentation typos and grammar
Issue 1281 - Fix indentation of parameters for class builbot
Issue 1281 - Document $adblockplus::buildmaster::project parameter
Issue 1281 - Rename adblockplus::buildserver to ::buildmaster
Issue 1281 - Remove example buildserver slaves from Hiera
Issue 1281 - Merge branch 'upstream' into 1281-buildbot
https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... File modules/buildbot/manifests/init.pp (right): https://codereview.adblockplus.org/29325436/diff/29326009/modules/buildbot/ma... modules/buildbot/manifests/init.pp:65: class buildbot ( On 2015/10/02 16:48:48, Felix Dahlke wrote: > I'm solely questioning why $*_config etc. are parameters, rather than variables. Acknowledged.
Sorry for the delay, here's another round of comments/questions! https://codereview.adblockplus.org/29325436/diff/29329287/hiera/roles/buildma... File hiera/roles/buildmasterserver.yaml (right): https://codereview.adblockplus.org/29325436/diff/29329287/hiera/roles/buildma... hiera/roles/buildmasterserver.yaml:3: domain: buildbot.adblockplus.org I'm wondering if build.adblockplus.org wouldn't be better. That's more in line with our other purpose-specific URLs (issues.adblockplus, codereview.adblockplus.org etc.), and it seems to be the most popular choice among prominent Buildbot users (such as Chromium and WebKit), too. https://codereview.adblockplus.org/29325436/diff/29329287/modules/adblockplus... File modules/adblockplus/files/nginx/buildmaster.conf (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/adblockplus... modules/adblockplus/files/nginx/buildmaster.conf:1: # Forward requests to the local builbot master Am I missing something, or does modules/adblockplus/files/nginx/buildmaster.conf not actually exist in the repository? https://codereview.adblockplus.org/29325436/diff/29329287/modules/adblockplus... File modules/adblockplus/manifests/buildmaster.pp (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/adblockplus... modules/adblockplus/manifests/buildmaster.pp:16: # translats directly into the $buildbot::master::project option. Typo: "translats" https://codereview.adblockplus.org/29325436/diff/29329287/modules/adblockplus... modules/adblockplus/manifests/buildmaster.pp:30: # Name => password pairs of e.g. remote build slaves. Why "e.g.", do we need credentials for local slaves? (Which we should probably not use to keep the master responsive.) https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... File modules/buildbot/manifests/init.pp (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/init.pp:63: @concat::fragment { So, do I understand it correctly that using concat for these allows us to have multiple masters/slaves on a single host? Is there a use case for that? https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... File modules/buildbot/manifests/master.pp (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/master.pp:22: # Whether to setup the master (anything but 'absent' or 'purged') or Typo: "set up" https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/master.pp:30: # [*project*] Wouldn't it be simpler to have two separate parameters for title and url rather than a hash containing both? https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/master.pp:35: # Local buildbot::slave records to setup with the master. Typo: "set up" https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... File modules/buildbot/manifests/slave.pp (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/slave.pp:9: # if it's ancestors are present, relied upon the builtin defaults. Typo: "its" https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/slave.pp:12: # Whether to setup the slave (anything but 'absent' or 'purged') or Typo: "set up" https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... File modules/buildbot/templates/master.cfg.erb (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... modules/buildbot/templates/master.cfg.erb:123: c['buildbotURL'] = 'http://localhost:<%= @http_port.to_i %>/' Shouldn't the host/domain/URL come from the manifest, rather than sticking with localhost here?
https://codereview.adblockplus.org/29325436/diff/29329287/hiera/roles/buildma... File hiera/roles/buildmasterserver.yaml (right): https://codereview.adblockplus.org/29325436/diff/29329287/hiera/roles/buildma... hiera/roles/buildmasterserver.yaml:3: domain: buildbot.adblockplus.org On 2015/11/19 19:50:00, Felix Dahlke wrote: > I'm wondering if http://build.adblockplus.org wouldn't be better. That's more in line > with our other purpose-specific URLs (issues.adblockplus, > http://codereview.adblockplus.org etc.), and it seems to be the most popular choice > among prominent Buildbot users (such as Chromium and WebKit), too. Acknowledged; but I'll use the plural form ("builds"). https://codereview.adblockplus.org/29325436/diff/29329287/modules/adblockplus... File modules/adblockplus/files/nginx/buildmaster.conf (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/adblockplus... modules/adblockplus/files/nginx/buildmaster.conf:1: # Forward requests to the local builbot master On 2015/11/19 19:50:00, Felix Dahlke wrote: > Am I missing something, or does modules/adblockplus/files/nginx/buildmaster.conf > not actually exist in the repository? It seems like an entity in the review-tool-chain (SCM/diff, upload.py or Rietveld) assumes that this file is related to modules/rietveld/files/site.conf - simply because they are almost identical - and thus diff's against it. Still, they have nothing to do with each other, so this is just a display issue here. https://codereview.adblockplus.org/29325436/diff/29329287/modules/adblockplus... File modules/adblockplus/manifests/buildmaster.pp (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/adblockplus... modules/adblockplus/manifests/buildmaster.pp:16: # translats directly into the $buildbot::master::project option. On 2015/11/19 19:50:00, Felix Dahlke wrote: > Typo: "translats" Acknowledged. https://codereview.adblockplus.org/29325436/diff/29329287/modules/adblockplus... modules/adblockplus/manifests/buildmaster.pp:30: # Name => password pairs of e.g. remote build slaves. On 2015/11/19 19:50:01, Felix Dahlke wrote: > Why "e.g.", do we need credentials for local slaves? Yes. > (Which we should probably not use to keep the master responsive.) Acknowledged. Still, it is possible, so the "e.g." should be fine. https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... File modules/buildbot/manifests/init.pp (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/init.pp:63: @concat::fragment { On 2015/11/19 19:50:01, Felix Dahlke wrote: > So, do I understand it correctly that using concat for these allows us to have > multiple masters/slaves on a single host? Yes we can have multiple masters and slaves per host. But this is not due to the use of concat, it's a feature in the software. The use of concat is a consequence from integrating this feature. > Is there a use case for that? For the master, in our case, probably not. For the slaves there are quite some scenarios, i.e. when a single host is used to operate multiple slaves that have individual privileges and are bound to individual build chains. Note, however, that module buildbot is meant to integrate the software itself, while module adblockplus::buildmaster contains the ABP specific stuff. https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... File modules/buildbot/manifests/master.pp (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/master.pp:22: # Whether to setup the master (anything but 'absent' or 'purged') or On 2015/11/19 19:50:01, Felix Dahlke wrote: > Typo: "set up" Acknowledged. https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/master.pp:30: # [*project*] On 2015/11/19 19:50:01, Felix Dahlke wrote: > Wouldn't it be simpler to have two separate parameters for title and url rather > than a hash containing both? Yes, it would be simpler. Though the idea is that any other project-related-meta-parameter can become a part of this hash, instead of introducing a Puppet and Hiera parameter for every single item. Also, the other parameters here are all in the system or admin domain, and I prefer not to mix them up with a bunch of parameters that are merely irrelevant in this context. However, giving this a second thought I figure it's not optimal having to document recognized (and possibly re-named) keys in plain text without semantics. The better idea is probably using the hash itself to initialize the BuildmasterConfig ("c"), as-is, without name adjustments and dry, without explicit per-option documentation (it's explained in the *.cfg anyway). https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/master.pp:35: # Local buildbot::slave records to setup with the master. On 2015/11/19 19:50:01, Felix Dahlke wrote: > Typo: "set up" Acknowledged. https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... File modules/buildbot/manifests/slave.pp (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/slave.pp:9: # if it's ancestors are present, relied upon the builtin defaults. On 2015/11/19 19:50:01, Felix Dahlke wrote: > Typo: "its" Acknowledged. https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/ma... modules/buildbot/manifests/slave.pp:12: # Whether to setup the slave (anything but 'absent' or 'purged') or On 2015/11/19 19:50:02, Felix Dahlke wrote: > Typo: "set up" Acknowledged. https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... File modules/buildbot/templates/master.cfg.erb (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... modules/buildbot/templates/master.cfg.erb:123: c['buildbotURL'] = 'http://localhost:<%= @http_port.to_i %>/' On 2015/11/19 19:50:02, Felix Dahlke wrote: > Shouldn't the host/domain/URL come from the manifest, rather than sticking with > localhost here? Indeed, the documentation (taken from the original sample.cfg) is a bit confusing: "internal web server" but "externally-visible", for example. I dug through the Python code to find some clarity. There is no actual use of the value, except for passing it on when generating status mails (though not explicitly used there either). Together with the fact that it's in the PROJECT IDENTITY section, I suppose my former interpretation ("internal web server") is wrong and this should be the base URI for external links ("externally-visible host name"). Note that this means the buildbotURL value can become another meta-parameter.
Did more of a high level review now, looks good mostly. Just two comments at this stage. https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... File modules/buildbot/templates/master.cfg.erb (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... modules/buildbot/templates/master.cfg.erb:123: c['buildbotURL'] = 'http://localhost:<%= @http_port.to_i %>/' On 2016/01/20 16:13:53, mathias wrote: > On 2015/11/19 19:50:02, Felix Dahlke wrote: > > Shouldn't the host/domain/URL come from the manifest, rather than sticking > with > > localhost here? > > Indeed, the documentation (taken from the original sample.cfg) is a bit > confusing: "internal web server" but "externally-visible", for example. > > I dug through the Python code to find some clarity. There is no actual use of > the value, except for passing it on when generating status mails (though not > explicitly used there either). Together with the fact that it's in the PROJECT > IDENTITY section, I suppose my former interpretation ("internal web server") is > wrong and this should be the base URI for external links ("externally-visible > host name"). Note that this means the buildbotURL value can become another > meta-parameter. Yeah, it's the externally visible URL. We're still not setting it properly how I see it. https://codereview.adblockplus.org/29325436/diff/29334156/modules/buildbot/te... File modules/buildbot/templates/master.cfg.erb (right): https://codereview.adblockplus.org/29325436/diff/29334156/modules/buildbot/te... modules/buildbot/templates/master.cfg.erb:115: c.setdefault('titleURL', 'http://example.com/') I suppose I'm missing something, but where are setting title and titleURL to their real values?
New patch-set will follow soon. https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... File modules/buildbot/templates/master.cfg.erb (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... modules/buildbot/templates/master.cfg.erb:123: c['buildbotURL'] = 'http://localhost:<%= @http_port.to_i %>/' On 2016/02/10 09:26:10, Felix Dahlke wrote: > On 2016/01/20 16:13:53, mathias wrote: > > On 2015/11/19 19:50:02, Felix Dahlke wrote: > > > Shouldn't the host/domain/URL come from the manifest, rather than sticking > > with > > > localhost here? > > > > Indeed, the documentation (taken from the original sample.cfg) is a bit > > confusing: "internal web server" but "externally-visible", for example. > > > > I dug through the Python code to find some clarity. There is no actual use of > > the value, except for passing it on when generating status mails (though not > > explicitly used there either). Together with the fact that it's in the PROJECT > > IDENTITY section, I suppose my former interpretation ("internal web server") > is > > wrong and this should be the base URI for external links ("externally-visible > > host name"). Note that this means the buildbotURL value can become another > > meta-parameter. > > Yeah, it's the externally visible URL. We're still not setting it properly how I > see it. Indeed, this should be a setdefault() as well (the URL is set in adblockplus::buildmaster). https://codereview.adblockplus.org/29325436/diff/29334156/modules/buildbot/te... File modules/buildbot/templates/master.cfg.erb (right): https://codereview.adblockplus.org/29325436/diff/29334156/modules/buildbot/te... modules/buildbot/templates/master.cfg.erb:115: c.setdefault('titleURL', 'http://example.com/') On 2016/02/10 09:26:10, Felix Dahlke wrote: > I suppose I'm missing something, but where are setting title and titleURL to > their real values? Above, line 14; c = BuildmasterConfig = <%= @config.to_json %>
https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... File modules/buildbot/templates/master.cfg.erb (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... modules/buildbot/templates/master.cfg.erb:123: c['buildbotURL'] = 'http://localhost:<%= @http_port.to_i %>/' On 2016/02/10 10:30:45, mathias wrote: > On 2016/02/10 09:26:10, Felix Dahlke wrote: > > On 2016/01/20 16:13:53, mathias wrote: > > > On 2015/11/19 19:50:02, Felix Dahlke wrote: > > > > Shouldn't the host/domain/URL come from the manifest, rather than sticking > > > with > > > > localhost here? > > > > > > Indeed, the documentation (taken from the original sample.cfg) is a bit > > > confusing: "internal web server" but "externally-visible", for example. > > > > > > I dug through the Python code to find some clarity. There is no actual use > of > > > the value, except for passing it on when generating status mails (though not > > > explicitly used there either). Together with the fact that it's in the > PROJECT > > > IDENTITY section, I suppose my former interpretation ("internal web server") > > is > > > wrong and this should be the base URI for external links > ("externally-visible > > > host name"). Note that this means the buildbotURL value can become another > > > meta-parameter. > > > > Yeah, it's the externally visible URL. We're still not setting it properly how > I > > see it. > > Indeed, this should be a setdefault() as well (the URL is set in > adblockplus::buildmaster). Waiting for another patch set with this then. https://codereview.adblockplus.org/29325436/diff/29334156/modules/buildbot/te... File modules/buildbot/templates/master.cfg.erb (right): https://codereview.adblockplus.org/29325436/diff/29334156/modules/buildbot/te... modules/buildbot/templates/master.cfg.erb:115: c.setdefault('titleURL', 'http://example.com/') On 2016/02/10 10:30:45, mathias wrote: > On 2016/02/10 09:26:10, Felix Dahlke wrote: > > I suppose I'm missing something, but where are setting title and titleURL to > > their real values? > > Above, line 14; c = BuildmasterConfig = <%= @config.to_json %> Acknowledged.
https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... File modules/buildbot/templates/master.cfg.erb (right): https://codereview.adblockplus.org/29325436/diff/29329287/modules/buildbot/te... modules/buildbot/templates/master.cfg.erb:123: c['buildbotURL'] = 'http://localhost:<%= @http_port.to_i %>/' On 2016/02/10 11:54:39, Felix Dahlke wrote: > Waiting for another patch set with this then. Actually this was correct in the last patch-set already, though the Rietveld GUI doesn't really make that obvious and I missed it myself.. However, see https://codereview.adblockplus.org/29325436/diff2/29329287:29334156/modules/b...
LGTM 
|
