Somewhere in the _verify_advisory_rpms_in_container_build(...), we should check for conf.dry_run and return True in case this conf.dry_run is set to True.

We use DRY_RUN = True on dev/qe freshmaker deployments as first level of freshmaker testing.

Maybe we need to handle potential exceptions here, for example 404, or others.

This line looks redundant and could be removed.

Suggest to move get rpms in an advisory to class Errata, which is something like Errata.get_advisory_rpms(errata_id).

This method actually does some kind of "diff" of rpms between an advisory and container. And this could be one criterion of verifying a container build, like this

def verify_a_container_build:
    if any_rpm_has_no_newer_version_in_container:
        fail
    if other_test_fails:
        fail

So, renaming _verify_advisory_rpms_in_container_build to another name that reflects what method does should make it more extensible to add more tests criteria in the future for container verification.

This PR for finding out if a rebuilt container has new version of RPMs from advisory. But this method only checks rpm names. Anything I missed?

rpm not in components checks whether the rpm from advisory is present in container.

this check only applies to container builds triggered by advisory change, advisory id is required for the checking, so I just keep it here.

Updated to:
1. address the dry_run case
2. build_id in db is actually task id in build system, so need to get build id from task info first. Fixed this.

I'll try to find an advisory to test this, and update it to add some improvements to address cqi's comments.

rpm not in components checks whether the rpm from advisory is present in container.

this check only applies to container builds triggered by advisory change, advisory id is required for the checking, so I just keep it here.

@qwan Updating PR causes these two comments' context is lost in Files Changed tab. :( Which comments do these two comments reply to?

rpm not in components checks whether the rpm from advisory is present in container.

This is for your comment of "But this method only checks rpm names. Anything I missed?"

this check only applies to container builds triggered by advisory change, advisory id is required for the checking, so I just keep it here.

This is for your suggestion of moving "_verify_advisory_rpms_in_container_build" to a more general function like "verify_a_container_build".

@qwan Updating PR causes these two comments' context is lost in Files Changed tab. :( Which comments do these two comments reply to?

On Wed, 2018-01-10 at 05:22 +0000, Qixiang Wan wrote:

qwan commented on the pull-request: Verify the rebuilt container has new version of RPMs from advisory that you are following:
``

rpm not in components checks whether the rpm from advisory is
present in container.

This is for your comment of "But this method only checks rpm names.
Anything I missed?"

this check only applies to container builds triggered by advisory
change, advisory id is required for the checking, so I just keep it
here.

This is for your suggestion of moving
"_verify_advisory_rpms_in_container_build" to a more general function
like "verify_a_container_build".

No. I meant renaming _verify_advisory_rpms_in_container_build to a
more specific name. verify_a_container_build I mentioned would be
another method which could call a set of "test methods" to verify a
rebuilt container.

@qwan Updating PR causes these two comments' context is lost in
Files Changed tab. :( Which comments do these two comments reply
to?

``

To reply, visit the link below or just reply to this email
https://pagure.io/freshmaker/pull-request/181

Rebased to address some comments from cqi,
@cqi, _verify_advisory_rpms_in_container_build is not changed, I'll look further into this to evaluate it later and open new PR if necessary.

You should limit only those components with "type": "rpm" - there can be others which would mean some fields wouldn't exist

updated get_rpms_in_container to limit only components with type 'rpm'.

+1 Looks good.