Skip to content

Commit

Permalink
frontend: unify naming convention for safe methods
Browse files Browse the repository at this point in the history
Originally, safe methods were considered as methods raising
ObjectNotFound for API (mainly with session.one() method). Now
methods which returns None or ignores the error completely
are also considered as "safe".

For the sake of consistency:
- *_noop_safe -> methods that ignores error completely (they basically
  logs)
- *_none_safe -> returns None if object was not found in the database
- *_safe -> raises an exception (ObjectNotFound) if no object found
  • Loading branch information
nikromen committed Nov 27, 2023
1 parent e4f544d commit 601a876
Show file tree
Hide file tree
Showing 11 changed files with 24 additions and 24 deletions.
4 changes: 2 additions & 2 deletions backend/run/copr_print_results_to_delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@ def main():
for chroot in os.listdir(projectpath):
if chroot in ["srpm-builds", "modules", "repodata", "pubkey.gpg"]:
continue
if not is_outdated_to_be_deleted(get_chroot_safe(client, ownername, projectname, chroot)):
if not is_outdated_to_be_deleted(get_chroot_none_safe(client, ownername, projectname, chroot)):
continue
print(os.path.join(projectpath, chroot))
except NotADirectoryError as ex:
print(str(ex))


def get_chroot_safe(client, ownername, projectname, chrootname):
def get_chroot_none_safe(client, ownername, projectname, chrootname):
try:
return client.project_chroot_proxy.get(ownername=ownername, projectname=projectname, chrootname=chrootname)
except CoprNoResultException:
Expand Down
4 changes: 2 additions & 2 deletions common/copr_common/worker_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ def add_task(self, task):
task.priority)
self.tasks.add_task(task, task.priority)

def _drop_task_id_safe(self, task_id):
def _drop_task_id_noop_safe(self, task_id):
try:
self.tasks.remove_task_by_id(task_id)
except KeyError:
Expand All @@ -400,7 +400,7 @@ def cancel_task_id(self, task_id):
:return: True if worker is running on background, False otherwise
"""
self._drop_task_id_safe(task_id)
self._drop_task_id_noop_safe(task_id)
worker_id = self.get_worker_id(task_id)
if worker_id not in self.worker_ids():
self.log.info("Cancel request, worker %s is not running", worker_id)
Expand Down
6 changes: 3 additions & 3 deletions dist-git/copr_dist_git/importer.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def post_back(self, data_dict):
log.debug("Sending back: \n{}".format(json.dumps(data_dict)))
return post(self.post_back_url, auth=self.auth, data=json.dumps(data_dict), headers=self.headers)

def post_back_safe(self, data_dict):
def post_back_noop_safe(self, data_dict):
"""
Ignores any error.
"""
Expand Down Expand Up @@ -98,7 +98,7 @@ def do_import(self, task):
shutil.rmtree(workdir)

log.info("sending a response for task {}".format(result))
self.post_back_safe(result)
self.post_back_noop_safe(result)
self.teardown_per_task_logging(per_task_log_handler)

def setup_per_task_logging(self, task):
Expand All @@ -120,7 +120,7 @@ def run(self):
worker_cls = Worker if self.opts.multiple_threads else SingleThreadWorker
self.is_running = True
while self.is_running:
pool.terminate_timeouted(callback=self.post_back_safe)
pool.terminate_timeouted(callback=self.post_back_noop_safe)
pool.remove_dead()

if pool.busy:
Expand Down
8 changes: 4 additions & 4 deletions dist-git/tests/test_importer.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,13 @@ def test_post_back(self, mc_post):

def test_post_back_safe(self, mc_post):
dd = {"foo": "bar"}
self.importer.post_back_safe(dd)
self.importer.post_back_noop_safe(dd)
assert mc_post.called
mc_post.reset_mock()
assert not mc_post.called

mc_post.side_effect = IOError
self.importer.post_back_safe(dd)
self.importer.post_back_noop_safe(dd)
assert mc_post.called

def test_do_import(self, mc_import_package, mc_helpers):
Expand All @@ -111,15 +111,15 @@ def test_do_import(self, mc_import_package, mc_helpers):
reponame='foo',
branch_commits={self.BRANCH: '123', self.BRANCH2: '124'}
)
self.importer.post_back_safe = MagicMock()
self.importer.post_back_noop_safe = MagicMock()
self.importer.do_import(self.url_task)

assert mc_import_package.call_args[0][0] == self.opts
assert mc_import_package.call_args[0][1] == self.url_task.repo_namespace
assert mc_import_package.call_args[0][2] == self.url_task.branches
assert mc_import_package.call_args[0][3] == 'somepath.src.rpm'

self.importer.post_back_safe.assert_has_calls([
self.importer.post_back_noop_safe.assert_has_calls([
mock.call({
'build_id': 123,
'pkg_name': 'foo',
Expand Down
4 changes: 2 additions & 2 deletions frontend/coprs_frontend/coprs/logic/builds_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ def get_pending_build_tasks(cls, background=None, data_type=None):
return query

@classmethod
def get_build_task(cls, task_id):
def get_build_task_safe(cls, task_id):
try:
build_id, chroot_name = task_id.split("-", 1)
except ValueError:
Expand Down Expand Up @@ -411,7 +411,7 @@ def get_multiple_by_copr(cls, copr):
def get_copr_builds_list(cls, copr, dirname=None):
query = models.Build.query.filter(models.Build.copr_id==copr.id)
if dirname:
copr_dir = coprs_logic.CoprDirsLogic.get_by_copr(copr, dirname)
copr_dir = coprs_logic.CoprDirsLogic.get_by_copr_safe(copr, dirname)
query = query.filter(models.Build.copr_dir_id==copr_dir.id)
query = query.options(selectinload('build_chroots'), selectinload('package'))
return query
Expand Down
2 changes: 1 addition & 1 deletion frontend/coprs_frontend/coprs/logic/complex_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ def get_build(build_id):
) from exc

@staticmethod
def get_build_chroot(build_id, chrootname):
def get_build_chroot_safe(build_id, chrootname):
"""
Get a BuildChroot instance based on build ID and name of the chroot.
Expand Down
6 changes: 3 additions & 3 deletions frontend/coprs_frontend/coprs/logic/coprs_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ def get_by_ownername(cls, ownername):
return cls.filter_by_ownername(query, ownername)

@classmethod
def get_by_ownername_coprname(cls, ownername, coprname):
def get_by_ownername_coprname_safe(cls, ownername, coprname):
"""
Return a Copr object owned by OWNERNAME (either '@groupname' or
'username') with a name COPRNAME.
Expand All @@ -206,7 +206,7 @@ def get_by_ownername_and_dirname(cls, ownername, dirname):
# Some of the calling methods (routes) strictly require us not to check
# if the dirname exists.
coprname = CoprDirsLogic.copr_name_from_dirname(dirname)
return cls.get_by_ownername_coprname(ownername, coprname)
return cls.get_by_ownername_coprname_safe(ownername, coprname)

@classmethod
def filter_without_ids(cls, query, ids):
Expand Down Expand Up @@ -632,7 +632,7 @@ def get_by_copr_or_none(cls, copr, dirname):
.filter(models.CoprDir.name==dirname)).first()

@classmethod
def get_by_copr(cls, copr, dirname):
def get_by_copr_safe(cls, copr, dirname):
"""
Return CoprDir instance per given Copr instance and dirname. Raise
ObjectNotFound if it doesn't exist.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def build_config(build_chroot):
@apiv3_ns.route("/build-chroot/<int:build_id>/<chrootname>", methods=GET) # deprecated
@query_params()
def get_build_chroot(build_id, chrootname):
chroot = ComplexLogic.get_build_chroot(build_id, chrootname)
chroot = ComplexLogic.get_build_chroot_safe(build_id, chrootname)
return flask.jsonify(to_dict(chroot))


Expand All @@ -62,7 +62,7 @@ def get_build_chroot_list(build_id, **kwargs):
@apiv3_ns.route("/build-chroot/build-config/<int:build_id>/<chrootname>", methods=GET) # deprecated
@query_params()
def get_build_chroot_config(build_id, chrootname):
chroot = ComplexLogic.get_build_chroot(build_id, chrootname)
chroot = ComplexLogic.get_build_chroot_safe(build_id, chrootname)
return flask.jsonify(build_config(chroot))


Expand All @@ -72,5 +72,5 @@ def get_build_chroot_built_packages(build_id, chrootname):
"""
Return built packages (NEVRA dicts) for a given build chroot
"""
chroot = ComplexLogic.get_build_chroot(build_id, chrootname)
chroot = ComplexLogic.get_build_chroot_safe(build_id, chrootname)
return flask.jsonify(chroot.results_dict)
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ def check_before_build():

# Raises an exception if CoprDir doesn't exist
if data["project_dirname"]:
CoprDirsLogic.get_by_copr(copr, data["project_dirname"])
CoprDirsLogic.get_by_copr_safe(copr, data["project_dirname"])

# Permissions check
if not flask.g.user.can_build_in(copr):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def package_monitor(ownername, projectname, project_dirname=None):
additional_fields = set()

if project_dirname:
copr_dir = CoprDirsLogic.get_by_copr(copr, project_dirname)
copr_dir = CoprDirsLogic.get_by_copr_safe(copr, project_dirname)
else:
copr_dir = copr.main_dir

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ def build_task_canceled(task_id):
if not was_running:
if '-' in task_id:
try:
build_chroot = BuildsLogic.get_build_task(task_id)
build_chroot = BuildsLogic.get_build_task_safe(task_id)
build_chroot.status = StatusEnum("canceled")
except ObjectNotFound:
pass
Expand Down Expand Up @@ -357,7 +357,7 @@ def _stream():
@backend_ns.route("/get-build-task/<task_id>")
def get_build_task(task_id):
try:
task = BuildsLogic.get_build_task(task_id)
task = BuildsLogic.get_build_task_safe(task_id)
build_record = get_build_record(task)
return flask.jsonify(build_record)
except CoprHttpException as ex:
Expand Down

0 comments on commit 601a876

Please sign in to comment.