Review entity methods, part 2

[abalakh]

Product:

• list_repositorysets() moved to RepositorySet.list_all()
• removed fetch_rhproduct_id() (use Product.search() instead)
• removed fetch_reposet_id() (use RepositorySet.search() instead)
• enable_rhrepo() now accepts payload dict instead of explicit parameters
• enable_rhrepo() moved to RepositorySet.enable()
• disable_rhrepo() now accepts payload dict instead of explicit parameters
• disable_rhrepo() moved to RepositorySet.disable()
• repository_sets_available_repositories() moved to RepositorySet.available_repositories()

Repository:

• Added ability to use search()
• removed fetch_repoid (use Repository.search() instead)
• upload_content() now accepts payload dict instead of explicit parameters

RepositorySet:

• Added new entity RepositorySet

RHCIDeployment:

• add_hypervisors() now accepts payload dict instead of explicit parameters
• deploy() now accepts payload dict instead of explicit parameters

SyncPlan:

• add_products() now accepts payload dict instead of explicit parameters
• remove_products() now accepts payload dict instead of explicit parameters

Updated unit tests with changes described above. Added tests for new RepositorySet entity.

[abalakh]

Part of #41

[sthirugn]

Can you paste the test results please?

[abalakh]

@sthirugn see SatelliteQE/robottelo#2632

[sthirugn]

Can you show me an example of enabling a repository after this change?

• enable_rhrepo() now accepts payload dict instead of explicit parameters
• enable_rhrepo() moved to RepositorySet.enable()

[abalakh]

If you use utils.enable_rhrepo_and_fetchid() - no changes. Function logic has changed, but it's still accepting and returning the same.

If you're working without that helper func, before you had to do smth like this:

    # Fetch product id by its name
    prd_id = entities.Product().fetch_rhproduct_id(
        name='Red Hat Enterprise Linux Server',
        org_id=org_id
    )
    # Fetch reposet id by its name
    reposet_id = entities.Product(id=prd_id).fetch_reposet_id(
        name='Red Hat Satellite Tools 6 Beta (for RHEL 7 Server) (RPMs)')
    # Enable RH repo
    entities.Product(id=prd_id).enable_rhrepo(
        base_arch='x86_64',
        release_ver='7Server',
        reposet_id=reposet_id,
    )

And now you should use search() to find entities:

    # Find product by name, save its ID
    prd_id = entities.Product(
        name='Red Hat Enterprise Linux Server',
        organization=org_id,
    ).search()[0].id
    # Find repository set by name
    reposet = entities.RepositorySet(
        name='Red Hat Satellite Tools 6 Beta (for RHEL 7 Server) (RPMs)',
        product=prd_id,
    ).search()[0]
    # Enable RH repo
    reposet.enable({'basearch': 'x86_64', 'releasever': '7Server'})

As you can see, enable() doesn't require reposet_id anymore (as now enable() is part of RepositorySet entity), and, according to the new nailgun helper functions approach (discussion here - #41) you should wrap all the arguments in a single dict instead of specifying each one individually.

[sthirugn]

Accepting only payloads vs attributes in some key methods will add more work to robottelo/sat users. Example:

To enable a repo:

Before this change:

entities.Product(id=prd_id).enable_rhrepo(base_arch='x86_64',release_ver=u'6.7',reposet_id=reposet_id)

After this change:

reposet = entities.RepositorySet(name=reposet, product=prd_id).search()[0]
reposet.enable({'basearch': basearch, 'releasever': releasever})

Problems:

1. I understand the search part of the changed code. But as a robottelo user, I am now expected to refer api doc to lookup attributes like 'basearch', 'releasever', etc.
2. In future if 'releasever' is changed to 'release_ver' then all individual references in robottelo needs to be fixed. Prior this change we would have needed to fix this in one place in nailgun function

[abalakh]

That wasn't fair comparison 😄 If you know the IDs, then you don't have to use search():

# 103
entities.Product(id=prd_id).enable_rhrepo(base_arch='x86_64', release_ver='6.7', reposet_id=reposet_id)
# 105
entities.RepositorySet(id=reposet_id, product=prd_id).enable({'basearch': 'x86_64', 'releasever': '6.7'})

103 vs 105 characters. Not that big change, and, basically, you should write both commands in 2 lines anyways.

1. Right, that's the drawback. At least, there will be no more mess with attributes naming (e.g. base_arch in nailgun and basearch in API) and any python-reserved words now can be used as attribute (e.g. type).
2. Basically, that was one of main ideas - to make nailgun more independent of API changes. No more changes to naigun are required if the set of attributes was changed. And it's easier to fix the same calls in robottelo than write the fix for different set of attributes for different product versions in nailgun. Just one quote about maintenance of approach with explicit parameters:

Imagine that the structure of arguments accepted by the server changes. In this case, a version comparison must be done in the method and each possible layout must be listed.

Imagine that the set of arguments accepted by the server changes. In this case, some painful hack must be performed, such as defining several nearly identical methods and binding one of them to an object at instantiation-time based on the server software version.

You can read more about why this approach was selected here, Jeremy perfectly described all the pros and cons of each approach.

[sthirugn]

Yeah, I wasn't worried about the search part, I am just concerned about the usage of payloads.

ok I wasn't aware about the other issue/discussion until today. my bad. I am still reading about why you all chose the payloads approach.

[sthirugn]

ACK - Great work!

[abalakh]

Updated, added even more unit tests. Actually, all the entities starting from Product now have 100% coverage 😄

[elyezer]

ACK

[Ichimonji10]

Thank you, @abalakh. Good job.

Accepting only payloads vs attributes in some key methods will add more work to robottelo/sat users.

Your criticism is totally correct, @sthirugn. But after weighing the pros and cons, I think it's the best solution available. :/

[sthirugn]

@Ichimonji10: Quick update: We discussed this strategy in detail on last friday's meeting and we agreed as a team to go ahead with this strategy, so we are good :) The recommendation is to write some robottelo util functions to overcome issues (mentioning api attribute names in each nailgun call) which this strategy brings to robottelo users.

[Ichimonji10]

@sthirugn Awesome. Sounds good.