Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1580#discussion_r89534499
--- Diff: tools/marvin/marvin/lib/base.py ---
@@ -3377,6 +3377,40 @@ def list(cls, apiclient, **kwargs):
cmd.listall = True
return(apiclient.listPortableIpRanges(cmd))
+
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
