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. ---