On Wed, Mar 29, 2017 at 11:48 PM, Andrew Kofink <[email protected]> wrote: > Hello CLI maintainers, > > (TL;DR I'm tired of broken tests in hammer-cli-katello due to hammer-cli > changes) > > We have a complex dependency structure between our CLI libraries > (hammer-cli, hammer-cli-foreman, hammer-cli-katello, csv, admin, etc.), and > it's not necessarily immune to changes in lower projects (i.e. hammer-cli). > An example is this recent PR > (https://github.com/theforeman/hammer-cli/pull/233) that slightly changes > the options/all_options methods in the abstract command. This caused > failures in hammer-cli-katello where we override/extend those methods, and I > had to open a PR to address the changes. > > I'd like to start a discussion on ways to keep this from happening in the > future. Some solutions I've thought of include (1) mentioning people on PRs > that have the potential to break dependent projects and (2) running > dependent projects' tests when a PR is submitted. > > (1) is not very robust. People can easily forget to mention others, and it's > difficult to remember all the appropriate people to mention. >
I'm afraid this isn't very robust indeed. I usually send a PR into plugins when I discover the change could break them. The problem is in the other cases when you think your change is fine (like the last time, I didn't expect hammer_cli_katello would be overriding the all_options method). > (2) seems like more of a pain for maintainers of the lower projects > (hammer-cli). If we went with this, it would still be up to the dependent > projects to update to adapt to the changes in hammer-cli. It has the benefit > of immediately alerting the maintainers that a breaking change is incoming. > Overall, the hammer tests run in seconds; I think hammer-cli-foreman is the > longest-running at ~120 seconds. hammer-cli runs in less than a second, and > hammer-cli-katello runs in about 5 seconds. I agree that adding the plugin tests would be good but they should be a warning that you changed something and you should notify plugin maintainers/consider sending patch rather than error that would block the merge forever. > > Let me know what you think! > > - Andrew > > -- > Andrew Kofink > [email protected] > IRC: akofink > Associate Software Engineer > Red Hat Satellite > > -- > You received this message because you are subscribed to the Google Groups > "foreman-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups "foreman-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
