Having just read through the class, +1 as well. On Wed, Jun 10, 2015 at 6:11 AM, Sam Corbett <[email protected]> wrote:
> Strongly agree. The "todo: this is now very bad code" has been there much > too long. > > On 10 June 2015 at 09:35, Andrea Turli <[email protected]> > wrote: > > > +1 > > > > On Wed, 10 Jun 2015 at 04:20 Martin Harris < > > [email protected]> > > wrote: > > > > > +1 > > > On 10 Jun 2015 05:10, "Aled Sage" <[email protected]> wrote: > > > > > > > Hi all, > > > > > > > > I'd like to do a major refactor of our class > > > > *brooklyn.location.jclouds.JcloudsLocation*. However, I suggest we > try > > to > > > > get a 0.7.0 release soon, and then do this refactoring. > > > > > > > > _*Current issues*_ > > > > The class has grown very big (2642 lines, including comments and > white > > > > space). > > > > > > > > The class has many responsibilities, which include: > > > > > > > > * VM creation > > > > * jclouds Template / TemplateOptions creation, for tailoring what VM > > > > to create > > > > * Retries and concurrency control, for VM creation > > > > * Post-processing of VM (e.g. extracting most appropriate ip/port) > > > > * VM customisation - for user creation > > > > * VM customisation - other, e.g. generating/setting hostname, open > > > > iptables, populating ~/.ssh/authorizedKeys, etc. > > > > * JcloudsMachineLocation construction (to represent the created VM) > > > > > > > > The class is hard to customise. There is a > > > > brooklyn.location.jclouds.JcloudsLocationCustomizer, which provides > > hooks > > > > for customising the provisioning process - it has six callbacks, i.e. > > six > > > > interception points where the behaviour can be customised. These > cover > > > many > > > > use-cases, but there are other use-cases that require intercepting at > > > other > > > > points. > > > > > > > > Sub-classing is poorly documented and potentially brittle. For > example, > > > in > > > > advanced-networking [1], it sub-classes JcloudsLocation to intercept > > > > behaviour at appropriate points to set up private networks / > > > > port-forwarding. We don't want to permanently support every protected > > > > method as part of a "public" API. We want a more thought-out approach > > to > > > > intercepting / augmenting behaviour. We want a clear statement about > > the > > > > backwards compatibility guarantees we'll give for sub-classing. > > > > > > > > There is quite complicated if-else behaviour. That would be very hard > > to > > > > remove entirely (e.g. user creation + credentials is controlled by > > > several > > > > config keys, which thus requires if-else). However it could likely be > > > > simplified. For example, windows versus linux differences could be > > > > extracted into separate classes - the decision is made once, and then > > the > > > > behaviour is different in each class. > > > > > > > > _*Proposal*_ > > > > We should separate out responsibilities into separate classes. We > > should > > > > use patterns, such as the "strategy", "command" and "factory" > patterns, > > > to > > > > inject behaviour. > > > > > > > > The injection should be customisable, so that power-users can > customise > > > > the behaviour (e.g. by replacing a given strategy class). > > > > > > > > Of the "responsibilities" listed above, some of those fit nicely into > > the > > > > refactoring (e.g. extract the "user creation" code into its own > class). > > > > Others map nicely to a cleaner separation of methods (e.g. > concurrency > > > > control not embedded in the middle of the method). > > > > Others fit into a general "post-provisioning customisation" phase, > > which > > > > could be its own class. > > > > > > > > _*Next steps*_ > > > > Let's get 0.7.0 GA out of the door first. > > > > > > > > I'm concious that the proposal above is vague, and a cynic could play > > > > buzz-word bingo with pattern names. > > > > > > > > However, if there's general agreement that this refactoring is worth > > > > while, then let's do some incremental improvements (e.g. starting > with > > > > extracting into separate classes), and do a detailed review in the > pull > > > > requests. > > > > > > > > Aled > > > > > > > > [1] https://github.com/brooklyncentral/advanced-networking > > > > > > > > > > > > > > > > > > -- > > > Cloudsoft Corporation Limited, Registered in Scotland No: SC349230. > > > Registered Office: 13 Dryden Place, Edinburgh, EH9 1RP > > > > > > This e-mail message is confidential and for use by the addressee only. > If > > > the message is received by anyone other than the addressee, please > return > > > the message to the sender by replying to it and then delete the message > > > from your computer. Internet e-mails are not necessarily secure. > > Cloudsoft > > > Corporation Limited does not accept responsibility for changes made to > > this > > > message after it was sent. > > > > > > Whilst all reasonable care has been taken to avoid the transmission of > > > viruses, it is the responsibility of the recipient to ensure that the > > > onward transmission, opening or use of this message and any attachments > > > will not adversely affect its systems or data. No responsibility is > > > accepted by Cloudsoft Corporation Limited in this regard and the > > recipient > > > should carry out such virus and other checks as it considers > appropriate. > > > > > > > -- > > Cloudsoft Corporation Limited, Registered in Scotland No: SC349230. > > Registered Office: 13 Dryden Place, Edinburgh, EH9 1RP > > > > This e-mail message is confidential and for use by the addressee only. If > > the message is received by anyone other than the addressee, please return > > the message to the sender by replying to it and then delete the message > > from your computer. Internet e-mails are not necessarily secure. > Cloudsoft > > Corporation Limited does not accept responsibility for changes made to > this > > message after it was sent. > > > > Whilst all reasonable care has been taken to avoid the transmission of > > viruses, it is the responsibility of the recipient to ensure that the > > onward transmission, opening or use of this message and any attachments > > will not adversely affect its systems or data. No responsibility is > > accepted by Cloudsoft Corporation Limited in this regard and the > recipient > > should carry out such virus and other checks as it considers appropriate. > > > > -- > Cloudsoft Corporation Limited, Registered in Scotland No: SC349230. > Registered Office: 13 Dryden Place, Edinburgh, EH9 1RP > > This e-mail message is confidential and for use by the addressee only. If > the message is received by anyone other than the addressee, please return > the message to the sender by replying to it and then delete the message > from your computer. Internet e-mails are not necessarily secure. Cloudsoft > Corporation Limited does not accept responsibility for changes made to this > message after it was sent. > > Whilst all reasonable care has been taken to avoid the transmission of > viruses, it is the responsibility of the recipient to ensure that the > onward transmission, opening or use of this message and any attachments > will not adversely affect its systems or data. No responsibility is > accepted by Cloudsoft Corporation Limited in this regard and the recipient > should carry out such virus and other checks as it considers appropriate. > -- Cloudsoft Corporation Limited, Registered in Scotland No: SC349230. Registered Office: 13 Dryden Place, Edinburgh, EH9 1RP This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the message from your computer. Internet e-mails are not necessarily secure. Cloudsoft Corporation Limited does not accept responsibility for changes made to this message after it was sent. Whilst all reasonable care has been taken to avoid the transmission of viruses, it is the responsibility of the recipient to ensure that the onward transmission, opening or use of this message and any attachments will not adversely affect its systems or data. No responsibility is accepted by Cloudsoft Corporation Limited in this regard and the recipient should carry out such virus and other checks as it considers appropriate.
