PR#511: Use extra->source instead of just source. - greenwave

greenwave

#511 Use extra->source instead of just source.

Merged 4 years ago by vmaljulin. Opened 4 years ago by vmaljulin.

vmaljulin/greenwave FACTORY-4898 into master

Use extra->source instead of just source.

Valerij Maljulin • 4 years ago

1923eb5

greenwave/resources.py

file modified

+9 -1

		`@@ -143,7 +143,15 @@`
		`if not build:`
		`raise NotFound('Failed to find Koji build for "{}" at "{}"'.format(nvr, koji_url))`

		`- source = build.get('source')`
		`+ source = None`
		`+ try:`
		`+ source = build['extra']['source']['original_url']`
		`+ except (TypeError, KeyError, AttributeError):`
		`+ pass`
		`+ finally:`
		`+ if not source:`
		`+ source = build.get('source')`
		`+`
		`if not source:`
		`raise NoSourceException(`
		`'Failed to retrieve SCM URL from Koji build "{}" at "{}" '`

greenwave/tests/test_policies.py

file modified

+1 -1

		`@@ -1053,7 +1053,7 @@`
		`with app.app_context():`
		`with mock.patch('xmlrpc.client.ServerProxy') as koji_server:`
		`koji_server_instance = mock.MagicMock()`
		`- koji_server_instance.getBuild.return_value = {'source': None}`
		`+ koji_server_instance.getBuild.return_value = {'extra': {'source': None}}`
		`koji_server.return_value = koji_server_instance`
		`policy = OnDemandPolicy.create_from_json(serverside_json)`

greenwave/tests/test_retrieve_gating_yaml.py

file modified

+34 -6

		`@@ -20,7 +20,27 @@`
		`nvr = 'nethack-3.6.1-3.fc29'`
		`build = {`
		`'nvr': nvr,`
		`- 'source': 'git+https://src.fedoraproject.org/rpms/nethack.git#0c1a84e0e8a152897003bd7e27b3f407ff6ba040' # noqa`
		`+ 'extra': {`
		`+ 'source': {`
		`+ 'original_url': 'git+https://src.fedoraproject.org/rpms/nethack.git#'`
		`+ '0c1a84e0e8a152897003bd7e27b3f407ff6ba040'`
		`+ }`
		`+ },`
		`+ # also check, that there's no fallback to source`
		`+ 'source': 'git+https://src.fedoraproject.org/rpms/nethack.git#master'`
		`+ }`
		`+ namespace, pkg_name, rev = retrieve_scm_from_koji_build(nvr, build, KOJI_URL)`
		`+ assert namespace == 'rpms'`
		`+ assert rev == '0c1a84e0e8a152897003bd7e27b3f407ff6ba040'`
		`+ assert pkg_name == 'nethack'`
		`+`
		`+`
		`+ def test_retrieve_scm_from_rpm_build_fallback_to_source():`
		`+ nvr = 'nethack-3.6.1-3.fc29'`
		`+ build = {`
		`+ 'nvr': nvr,`
		`+ 'source': 'git+https://src.fedoraproject.org/rpms/nethack.git#'`
		`+ '0c1a84e0e8a152897003bd7e27b3f407ff6ba040'`
		`}`
		`namespace, pkg_name, rev = retrieve_scm_from_koji_build(nvr, build, KOJI_URL)`
		`assert namespace == 'rpms'`
		`@@ -32,7 +52,9 @@`
		`nvr = 'golang-github-openshift-prometheus-alert-buffer-container-v3.10.0-0.34.0.0'`
		`build = {`
		`'nvr': nvr,`
		`- 'source': 'git://pkgs.devel.redhat.com/containers/golang-github-openshift-prometheus-alert-buffer#46af2f8efbfb0a4e7e7d5676f4efb997f72d4b8c' # noqa`
		`+ 'source': 'git://pkgs.devel.redhat.com/containers/'`
		`+ 'golang-github-openshift-prometheus-alert-buffer#'`
		`+ '46af2f8efbfb0a4e7e7d5676f4efb997f72d4b8c'`
		`}`
		`namespace, pkg_name, rev = retrieve_scm_from_koji_build(nvr, build, KOJI_URL)`
		`assert namespace == 'containers'`
		`@@ -62,7 +84,11 @@`
		`nvr = 'foo-1.2.3-1.fc29'`
		`build = {`
		`'nvr': nvr,`
		`- 'source': 'git+https://src.fedoraproject.org/foo.git#deadbeef',`
		`+ 'extra': {`
		`+ 'source': {`
		`+ 'original_url': 'git+https://src.fedoraproject.org/foo.git#deadbeef'`
		`+ }`
		`+ }`
		`}`
		`namespace, pkg_name, rev = retrieve_scm_from_koji_build(nvr, build, KOJI_URL)`
		`assert namespace == ''`
		`@@ -74,7 +100,11 @@`
		`nvr = 'foo-1.2.3-1.fc29'`
		`build = {`
		`'nvr': nvr,`
		`- 'source': 'git+https://src.fedoraproject.org/rpms/foo.git',`
		`+ 'extra': {`
		`+ 'source': {`
		`+ 'original_url': 'git+https://src.fedoraproject.org/rpms/foo.git'`
		`+ }`
		`+ }`
		`}`
		`expected_error = 'missing URL fragment with SCM revision information'`
		`with pytest.raises(BadGateway, match=expected_error):`
		`@@ -103,8 +133,6 @@`
		`app = greenwave.app_factory.create_app()`
		`with app.app_context():`
		`with mock.patch('requests.Session.request') as mocked_request:`
		`- # Return 404, because we are only interested in the URL in the request`
		`- # and whether it is correct even with empty namespace.`
		`response = mock.MagicMock()`
		`response.status_code = 200`
		`mocked_request.side_effect = [`

vmaljulin commented 4 years ago

This fixes #470.

Signed-off-by: Valerij Maljulin vmaljuli@redhat.com

lholecek commented on line 10 of greenwave/resources.py 4 years ago

It should not be logged as an error since it's expected to work this way for containers.

lholecek commented 4 years ago

Can we have a test for some container data missing the extra.source field?

vmaljulin commented 4 years ago

Can we have a test for some container data missing the extra.source field?

It doesn't matter if it is container data or everything else. If it has no extra.source it will check the source as it was before (and this behavior is tested) regardless of other circumstances.

rebased onto 4dab05039fb4152b2f159d75644b0776fb75527e

4 years ago

vmaljulin commented 4 years ago

It should not be logged as an error since it's expected to work this way for containers.

changed to warning

lholecek commented 4 years ago

Even warning may be too obtrusive in logs. I don't want to see the message being printed for every container build since it's expected.

It might be better for now to just use extra.source for RPM builds and source for container builds -- they have not yet decided to support the new field for containers: https://github.com/containerbuildsystem/atomic-reactor/issues/1248