PR#1577: Check schedule date when adding new streams - fm-orchestrator

fm-orchestrator

#1577 Check schedule date when adding new streams

Merged 4 years ago by mikem. Opened 4 years ago by breilly.

breilly/fm-orchestrator enhance-zstream into master

extend unittest

Brendan Reilly • 4 years ago

2f75b5e

Check schedule date when adding new streams

Brendan Reilly • 4 years ago

88597a7

module_build_service/common/config.py

file modified

		`@@ -665,6 +665,12 @@`
		`"string that represent the Product Pages release for major releases "`
		`"(e.g. 8.0.0). After the first match, the rest will be ignored."`
		`},`
		`+ "product_pages_schedule_task_name": {`
		`+ "type": str,`
		`+ "default": "",`
		`+ "desc": "A schedule task name. This is used to check Product schedules for cases where "`
		`+ "the stream should be used before release."`
		`+ },`
		`"num_threads_for_build_submissions": {`
		`"type": int,`
		`"default": 5,`

module_build_service/web/submit.py

file modified

+76 -27

		`@@ -374,6 +374,70 @@`

		`buildrequire_overrides = params.get("buildrequire_overrides", {})`

		`+ def is_released_as_per_schedule(pp_release):`
		`+ """`
		`+ Check if the specified scheduled task date has been reached. Returns True if it has.`
		`+ """`
		`+ if not conf.product_pages_schedule_task_name:`
		`+ log.debug(config_msg, "product_pages_schedule_task_name")`
		`+ return False`
		`+`
		`+ schedule_url = "{}/api/v7/releases/{}/schedule-tasks/?fields=name,date_finish".format(`
		`+ conf.product_pages_url.rstrip("/"), pp_release)`
		`+`
		`+ try:`
		`+ pp_rv = requests.get(schedule_url, timeout=15)`
		`+ pp_json = pp_rv.json()`
		`+ # Catch requests failures and JSON parsing errors`
		`+ except (requests.exceptions.RequestException, ValueError):`
		`+ log.exception(`
		`+ "The query to the Product Pages at %s failed. Assuming it is not available.",`
		`+ schedule_url,`
		`+ )`
		`+ return False`
		`+`
		`+ name = conf.product_pages_schedule_task_name.lower().strip()`
		`+ for task in pp_json:`
		`+ if task['name'].lower().strip() == name:`
		`+ task_date = task['date_finish']`
		`+ if datetime.strptime(task_date, "%Y-%m-%d").date() >= datetime.utcnow().date():`
		`+ log.debug(`
		`+ "The task date %s hasn't been reached yet. Not adding a stream suffix.",`
		`+ task_date`
		`+ )`
		`+ return False`
		`+ return True`
		`+ # Schedule task not available; rely on GA date`
		`+ return False`
		`+`
		`+ def is_released(pp_release, url):`
		`+ """`
		`+ Check if the stream has been released. Return True if it has.`
		`+ """`
		`+ try:`
		`+ pp_rv = requests.get(url, timeout=15)`
		`+ pp_json = pp_rv.json()`
		`+ # Catch requests failures and JSON parsing errors`
		`+ except (requests.exceptions.RequestException, ValueError):`
		`+ log.exception(`
		`+ "The query to the Product Pages at %s failed. Assuming it is not yet released.",`
		`+ url,`
		`+ )`
		`+ return False`
		`+`
		`+ ga_date = pp_json.get("ga_date")`
		`+ if not ga_date:`
		`+ log.debug("A release date for the release %s could not be determined", pp_release)`
		`+ return False`
		`+`
		`+ if datetime.strptime(ga_date, "%Y-%m-%d").date() >= datetime.utcnow().date():`
		`+ log.debug(`
		`+ "The release %s hasn't been released yet. Not adding a stream suffix.",`
		`+ ga_date`
		`+ )`
		`+ return False`
		`+ return True`
		`+`
		`def new_streams_func(db_session, name, streams):`
		`if name not in conf.base_module_names:`
		`log.debug("The module %s is not a base module. Skipping the release date check.", name)`
		`@@ -435,35 +499,20 @@`
		`url = "{}/api/v7/releases/{}/?fields=ga_date".format(`
		`conf.product_pages_url.rstrip("/"), pp_release)`

		`- try:`
		`- pp_rv = requests.get(url, timeout=15)`
		`- pp_json = pp_rv.json()`
		`- # Catch requests failures and JSON parsing errors`
		`- except (requests.exceptions.RequestException, ValueError):`
		`- log.exception(`
		`- "The query to the Product Pages at %s failed. Assuming it is not yet released.",`
		`- url,`
		`+ if is_released_as_per_schedule(pp_release):`
		`+ new_stream = stream + stream_suffix`
		`+ log.info(`
		`+ 'Replacing the buildrequire "%s:%s" with "%s:%s", since the date is met',`
		`+ name, stream, name, new_stream`
		`)`
		`- continue`
		`-`
		`- ga_date = pp_json.get("ga_date")`
		`- if not ga_date:`
		`- log.debug("A release date for the release %s could not be determined", pp_release)`
		`- continue`
		`-`
		`- if datetime.strptime(ga_date, "%Y-%m-%d").date() >= datetime.utcnow().date():`
		`- log.debug(`
		`- "The release %s hasn't been released yet. Not adding a stream suffix.",`
		`- ga_date`
		`+ new_streams[i] = new_stream`
		`+ elif is_released(pp_release, url):`
		`+ new_stream = stream + stream_suffix`
		`+ log.info(`
		`+ 'Replacing the buildrequire "%s:%s" with "%s:%s", since the stream is released',`
		`+ name, stream, name, new_stream`
		`)`
		`- continue`
		`-`
		`- new_stream = stream + stream_suffix`
		`- log.info(`
		`- 'Replacing the buildrequire "%s:%s" with "%s:%s", since the stream is released',`
		`- name, stream, name, new_stream`
		`- )`
		`- new_streams[i] = new_stream`
		`+ new_streams[i] = new_stream`

		`return new_streams`

tests/test_web/test_views.py

file modified

+37 -5

		`@@ -2497,12 +2497,13 @@`
		`assert dep.get_runtime_streams("platform") == ["el8"]`

		`@pytest.mark.parametrize(`
		`- "pp_url, pp_streams, get_rv, br_stream, br_override, expected_stream, utcnow",`
		`+ "pp_url, pp_streams, pp_sched, get_rv, br_stream, br_override, expected_stream, utcnow",`
		`(`
		`# Test a stream of a major release`
		`(`
		`"https://pp.domain.local/pp/",`
		`{r"el.+": (".z", "rhel-{x}-{y}.{z}", "rhel-{x}-{y}")},`
		`+ None,`
		`{"ga_date": "2019-05-07"},`
		`"el8.0.0",`
		`{},`
		`@@ -2513,6 +2514,7 @@`
		`(`
		`"https://pp.domain.local/pp/",`
		`{r"el.+": (".z", "rhel-{x}-{y}.{z}", "rhel-{x}-{y}")},`
		`+ None,`
		`{"ga_date": "2099-10-30"},`
		`"el8.0.0",`
		`{},`
		`@@ -2523,6 +2525,7 @@`
		`(`
		`"https://pp.domain.local/pp/",`
		`{r"el.+": (".z", "rhel-{x}-{y}.{z}", "rhel-{x}-{y}")},`
		`+ None,`
		`{"ga_date": "2019-09-16"},`
		`"el8.0.0",`
		`{},`
		`@@ -2533,6 +2536,7 @@`
		`(`
		`"",`
		`{r"el.+": (".z", "rhel-{x}-{y}.{z}", "rhel-{x}-{y}")},`
		`+ None,`
		`{"ga_date": "2019-05-07"},`
		`"el8.0.0",`
		`{},`
		`@@ -2543,6 +2547,7 @@`
		`(`
		`"https://pp.domain.local/pp/",`
		`{r"el.+": (".z", "rhel-{x}-{y}.{z}", "rhel-{x}-{y}")},`
		`+ None,`
		`{"detail": "Not found."},`
		`"el8.0.0",`
		`{},`
		`@@ -2553,6 +2558,7 @@`
		`(`
		`"https://pp.domain.local/pp/",`
		`{r"el.+": (".z", "rhel-{x}-{y}.{z}", "rhel-{x}-{y}")},`
		`+ None,`
		`{"ga_date": "2019-05-07"},`
		`"el8.2.1",`
		`{},`
		`@@ -2563,6 +2569,7 @@`
		`(`
		`"https://pp.domain.local/pp/",`
		`{r"el.+": (".z", "rhel-{x}-{y}.{z}", "rhel-{x}-{y}")},`
		`+ None,`
		`{"ga_date": "2019-05-07"},`
		`"el8.0.0",`
		`{"platform": ["el8.0.0"]},`
		`@@ -2573,6 +2580,7 @@`
		`(`
		`"https://pp.domain.local/pp/",`
		`{},`
		`+ None,`
		`{"ga_date": "2019-05-07"},`
		`"el8.0.0",`
		`{},`
		`@@ -2583,6 +2591,7 @@`
		`(`
		`"https://pp.domain.local/pp/",`
		`{r"js.+": (".z", "js-{x}-{y}", "js-{x}-{y}")},`
		`+ None,`
		`{"ga_date": "2019-05-07"},`
		`"el8.0.0",`
		`{},`
		`@@ -2593,12 +2602,24 @@`
		`(`
		`"https://pp.domain.local/pp/",`
		`{r"el.+": (".z", "rhel-{x}-{y}", None)},`
		`+ None,`
		`{"ga_date": "2019-05-07"},`
		`"el8.0.0",`
		`{},`
		`"el8.0.0.z",`
		`datetime(2019, 9, 16, 12, 00, 00, 0),`
		`),`
		`+ # Test when there is a schedule date set for early release`
		`+ (`
		`+ "https://pp.domain.local/pp/",`
		`+ {r"el.+": (".z", "rhel-{x}-{y}", None)},`
		`+ "test_sched",`
		`+ [{"name": "test_sched", "date_finish": "2019-09-02"}],`
		`+ "el8.0.0",`
		`+ {},`
		`+ "el8.0.0.z",`
		`+ datetime(2019, 9, 16, 12, 00, 00, 0),`
		`+ ),`
		`),`
		`)`
		`@patch.object(`
		`@@ -2614,16 +2635,22 @@`
		`"module_build_service.common.config.Config.product_pages_module_streams",`
		`new_callable=PropertyMock,`
		`)`
		`+ @patch(`
		`+ "module_build_service.common.config.Config.product_pages_schedule_task_name",`
		`+ new_callable=PropertyMock,`
		`+ )`
		`@patch("requests.get")`
		`@patch("module_build_service.web.auth.get_user", return_value=user)`
		`@patch("module_build_service.common.scm.SCM")`
		`def test_submit_build_automatic_z_stream_detection(`
		`- self, mocked_scm, mocked_get_user, mock_get, mock_pp_streams, mock_pp_url, mock_datetime,`
		`- pp_url, pp_streams, get_rv, br_stream, br_override, expected_stream, utcnow,`
		`+ self, mocked_scm, mocked_get_user, mock_get, mock_pp_sched, mock_pp_streams, mock_pp_url,`
		`+ mock_datetime, pp_url, pp_streams, pp_sched, get_rv, br_stream, br_override,`
		`+ expected_stream, utcnow,`
		`):`
		`# Configure the Product Pages URL`
		`mock_pp_url.return_value = pp_url`
		`mock_pp_streams.return_value = pp_streams`
		`+ mock_pp_sched.return_value = pp_sched`
		`# Mock the Product Pages query`
		`mock_get.return_value.json.return_value = get_rv`
		`# Mock the date`
		`@@ -2660,12 +2687,17 @@`
		`# The runtime stream suffix should remain unchanged`
		`assert dep.get_runtime_streams("platform") == ["el8.0.0"]`

		`- if pp_url and not br_override and pp_streams.get(r"el.+"):`
		`+ if (pp_url or pp_sched) and not br_override and pp_streams.get(r"el.+"):`
		`if br_stream == "el8.0.0":`
		`pp_release = "rhel-8-0"`
		`else:`
		`pp_release = "rhel-8-2.1"`
		`- expected_url = "{}api/v7/releases/{}/?fields=ga_date".format(pp_url, pp_release)`
		`+ if pp_url:`
		`+ expected_url = "{}api/v7/releases/{}/?fields=ga_date".format(pp_url, pp_release)`
		`+ if pp_sched:`
		`+ expected_url = \`
		`+ "{}api/v7/releases/{}/schedule-tasks/?fields=name,date_finish".format(`
		`+ pp_url, pp_release)`
		`mock_get.assert_called_once_with(expected_url, timeout=15)`
		`else:`
		`mock_get.assert_not_called()`

no initial comment

factory2jenkins commented 4 years ago

Build #750 failed (commit: 34e1e4b3154f0af4ee930cda77666f1c3252ecdb).
Rebase or make new commits to rebuild.

mikem commented 4 years ago

+        pattern = conf.product_pages_schedule_task_name.lower().strip()
+        for task in pp_json:
+            if task['name'].lower().strip() == pattern:

The variable pattern is only used in a string equality test (modulo case and stripping), but that name suggests it could be a glob or regex pattern. Perhaps just name?

mikem commented 4 years ago

So now we have two different checks again the product_pages api in this function that are asking similar questions and taking what appears to be the same action when we get a true result. The one you'd added is handled by a local function, but the other is inline. Seems like it might be a little cleaner if the other one was also split into a function. Might make the logic a bit more apparent.

Edited 4 years ago by mikem

mikem commented 4 years ago

Also, we should probably extend test_submit_build_automatic_z_stream_detection to cover this behavior.

mikem commented 4 years ago

probably worth rebasing too