Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ART-9021 get rid of auto in canonical_builders_from_upstream #669

Merged
merged 1 commit into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 2 additions & 20 deletions artcommon/artcommonlib/build_util.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
from datetime import datetime
from typing import List, Optional, Iterable, Dict

from artcommonlib import logutil
from artcommonlib.model import Missing
from artcommonlib.release_util import isolate_assembly_in_release
from doozerlib.release_schedule import ReleaseSchedule

LOGGER = logutil.get_logger(__name__)

Expand Down Expand Up @@ -61,25 +58,10 @@ def canonical_builders_enabled(canonical_builders_from_upstream, runtime) -> boo
# Default case: override using ART's config
return False

elif canonical_builders_from_upstream == 'auto':
# canonical_builders_from_upstream set to 'auto': rebase according to release schedule
try:
feature_freeze_date = ReleaseSchedule(runtime).get_ff_date()
return datetime.now() < feature_freeze_date
except ChildProcessError:
# Could not access Gitlab: display a warning and fallback to default
LOGGER.warning('Failed retrieving release schedule from Gitlab: fallback to using ART\'s config')
return False
except ValueError as e:
# A GITLAB token env var was not provided: display a warning and fallback to default
LOGGER.warning(f'Fallback to default ART config: {e}')
return False

elif canonical_builders_from_upstream in ['on', True]:
# yaml parser converts bare 'on' to True, same for 'off' and False
elif canonical_builders_from_upstream is True:
return True

elif canonical_builders_from_upstream in ['off', False]:
elif canonical_builders_from_upstream is False:
return False

else:
Expand Down
49 changes: 0 additions & 49 deletions doozer/doozerlib/release_schedule.py

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -610,9 +610,7 @@ def test_generate_osbs_image_config_with_cachito_enabled(self, is_dir: Mock):
self.assertEqual(actual["remote_sources"][0]["remote_source"]["pkg_managers"], ["gomod"])
self.assertEqual(actual["remote_sources"][0]["remote_source"]["flags"], ["gomod-vendor-check"])

@patch('artcommonlib.build_util.datetime')
@patch('artcommonlib.build_util.ReleaseSchedule')
def test_canonical_builders_enabled(self, release_schedule, mock_datetime):
def test_canonical_builders_enabled(self):
# canonical_builders_from_upstream not defined
self.img_dg.config.canonical_builders_from_upstream = Missing
self.assertEqual(False, self.img_dg._canonical_builders_enabled())
Expand All @@ -627,23 +625,6 @@ def test_canonical_builders_enabled(self, release_schedule, mock_datetime):
self.img_dg.runtime.group_config.canonical_builders_from_upstream = True
self.assertEqual(False, self.img_dg._canonical_builders_enabled())

# canonical_builders_from_upstream = 'on' in image config (override)
Copy link
Contributor

Choose a reason for hiding this comment

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

This block should still apply, why removing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the on and off also need to be replaced with true and false, pr updated to limit supported mode to [ True, False ]

self.img_dg.config.canonical_builders_from_upstream = 'on'
self.img_dg.runtime.group_config.canonical_builders_from_upstream = False
self.assertEqual(True, self.img_dg._canonical_builders_enabled())

# canonical_builders_from_upstream = 'auto'; current time < feature freeze
self.img_dg.config.canonical_builders_from_upstream = 'auto'
release_schedule.return_value.get_ff_date.return_value = datetime.datetime(2023, 6, 1, 10, 30, 15)
mock_datetime.now.return_value = datetime.datetime(2023, 5, 1, 10, 30, 15)
self.assertEqual(True, self.img_dg._canonical_builders_enabled())

# canonical_builders_from_upstream = 'auto'; current time > feature freeze
self.img_dg.config.canonical_builders_from_upstream = 'auto'
release_schedule.return_value.get_ff_date.return_value = datetime.datetime(2023, 6, 1, 10, 30, 15)
mock_datetime.now.return_value = datetime.datetime(2023, 7, 1, 10, 30, 15)
self.assertEqual(False, self.img_dg._canonical_builders_enabled())

def test_update_image_config(self):
self.img_dg.metadata = MagicMock()
# 'when' clause matching upstream rhel version: override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,7 @@
},
"urls-": {},
"canonical_builders_from_upstream": {
"description": "Assert our base images, or listen to upstream",
"enum": [true, false, "auto", "off", "no", "on", "yes"]
Copy link
Contributor

Choose a reason for hiding this comment

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

The code still handles on and off, so why deprecating these in the schema? If we do so, the code should only allow True or False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the on and off also need to be replaced with true and false, let's limit the value to bool true and false

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so maybe we can use a boolean type instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, pr updated

"type": "boolean"
},
"canonical_builders_from_upstream?": {
"$ref": "#/properties/canonical_builders_from_upstream"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,7 @@
},
"content-": {},
"canonical_builders_from_upstream": {
"description": "Override the facility to adhere to upstream suggestions for base images",
"enum": [true, false, "auto", "off", "no", "on", "yes"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

"type": "boolean"
},
"canonical_builders_from_upstream?": {
"$ref": "#/properties/canonical_builders_from_upstream"
Expand Down
Loading