cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rhtyd <>
Subject [GitHub] cloudstack pull request #1580: CLOUDSTACK-9402 : Support for underlay featur...
Date Thu, 24 Nov 2016 17:52:21 GMT
Github user rhtyd commented on a diff in the pull request:
    --- Diff: tools/marvin/marvin/lib/ ---
    @@ -3377,6 +3377,40 @@ def list(cls, apiclient, **kwargs):
                 cmd.listall = True
    +class NuageUnderlayPublicIpRange:
    --- End diff --
    I understand it's easier that way and that's what others have been doing, but I disagree
here and this pattern need to stop.
    If you put the utility in the plugins/nuagevsp your nuage specific integration tests in
that directory can still import from the utility just as you import from marvin.base, this
I think will be much clearer implementation than keep adding resource/utility classes to the
base marvin utility. In fact, I think this module should not be part of Marvin at all. I've
similar thoughts about the test_data, a single source of test data consumed by all tests and
that too within Marvin.
    Adding more test-specific dependencies in Marvin will make it harder to split Marvin as
a standalone library. The library should be made independent of tests, and allow tests to
have their own test data and utilities. That said, I understand the accumulated technical
debt is nobody's fault, it just happened and we had no guideline or enforcement at the time.
    I still want to encourage best practices, and if there is lack of bandwidth I could still
help with the merge with hope to address this technical debt in a future refactoring round.

If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at or file a JIRA ticket
with INFRA.

View raw message