Skip to content
This repository has been archived by the owner on May 24, 2023. It is now read-only.

Greenwave integration #64

Merged
merged 4 commits into from
Apr 15, 2019
Merged

Conversation

MartinBasti
Copy link
Contributor

No description provided.

@MartinBasti MartinBasti requested a review from csomh April 12, 2019 14:59
@@ -134,6 +135,10 @@ def extract_zip_file_from_koji(
"""
with NamedTemporaryFile('wb', suffix='.zip') as tmpf:
KOJI.download_manifest_archive(nvr, tmpf)
# running KOJI methods first, it has checks that should pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this read: running GREENWAVE methods first, it has checks that should pass even before we bother koji?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you want to check greenwave first?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I was confused by the placement of the comment... I guess, should have been above the KOJI.download_... line... but now that you ask: I think the Greenwave check could go first. CVP checks will probably fail for similar reasons KOJI.download_... would fail. Even if not, probably it's better to download the content only if there is a chance to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed order

else:
try:
data = response.json()
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should only catch ValueErrors.

From requests.Response.json docs:

:raises ValueError: If the response body does not contain valid json.

omps/greenwave.py Outdated Show resolved Hide resolved
omps/greenwave.py Outdated Show resolved Hide resolved
* OSBS-7140

Signed-off-by: Martin Bašti <mbasti@redhat.com>
Signed-off-by: Martin Bašti <mbasti@redhat.com>
Signed-off-by: Martin Bašti <mbasti@redhat.com>
Signed-off-by: Martin Bašti <mbasti@redhat.com>
@MartinBasti
Copy link
Contributor Author

All comments should be resolved

@MartinBasti MartinBasti merged commit 6cb5e91 into release-engineering:master Apr 15, 2019
@MartinBasti MartinBasti deleted the greenwave branch April 15, 2019 12:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants