On Wed, Mar 29, 2017 at 11:48 PM, Andrew Kofink 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
> akof...@redhat.com
> 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 foreman-dev+unsubscr...@googlegroups.com.
> 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 foreman-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
