Refactor: New KonfluxClient class for calling Konflux APIs #1151
Conversation
vfreex
force-pushed
the
konflux-client
branch
from
2121e99 to
97c2023
Compare
|
/approve unittests need to be updated though |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashwindasr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@ashwindasr Thanks for the review. I've pushed another commit to fix the flake8 errors. Test build: https://art-jenkins.apps.prod-stable-spoke1-dc-iad2.itup.redhat.com/job/aos-cd-builds/job/build%252Focp4-konflux/362/console |
openshift-merge-robot
added
the
needs-rebase
vfreex
force-pushed
the
konflux-client
branch
from
63eb022 to
4f6a513
Compare
openshift-merge-robot
removed
the
needs-rebase
vfreex
force-pushed
the
konflux-client
branch
from
4f6a513 to
d7fb85b
Compare
| async def _patch(self, manifest: dict): | ||
| """ Patch a resource. | ||
|
|
||
| :param dyn_client: The dynamic client. |
There was a problem hiding this comment.
param dyn_client is not defined
| async def _replace(self, manifest: dict): | ||
| """ Replace a resource. | ||
|
|
||
| :param dyn_client: The dynamic client. |
There was a problem hiding this comment.
param dyn_client is not defined
| async def _delete(self, api_version: str, kind: str, name: str, namespace: str): | ||
| """ Delete a resource. | ||
|
|
||
| :param dyn_client: The dynamic client. |
There was a problem hiding this comment.
param dyn_client is not defined
| async def _create_or_patch(self, manifest: dict): | ||
| """ Create or patch a resource. | ||
|
|
||
| :param dyn_client: The dynamic client. |
There was a problem hiding this comment.
param dyn_client is not defined
| async def _create_or_replace(self, manifest: dict): | ||
| """ Create or replace a resource. | ||
|
|
||
| :param dyn_client: The dynamic client. |
There was a problem hiding this comment.
param dyn_client is not defined
| output_image: str, | ||
| building_arches: Sequence[str], | ||
| git_auth_secret: str = "pipelines-as-code-secret", | ||
| additional_tags: Sequence[str] = [], |
There was a problem hiding this comment.
default value for additional_tags sis mutable. In Python, mutable args are evaluated only once at function definition time, so calling the function with a different value of the argument will affect all subsequent calls of that function. Better use None instead
| """ | ||
| Wait for a PipelineRun to complete. | ||
|
|
||
| :param dyn_client: The dynamic client. |
There was a problem hiding this comment.
param dyn_client is not defined
| if e.status == 410: | ||
| # If the last result is too old, an `ApiException` exception will be thrown with | ||
| # `code` 410. In that case we have to recover by retrying without resource_version. | ||
| self._logger.debug("%s: Resource version %s is too old. Recovering...", pipelinerun_name, resource_version) |
There was a problem hiding this comment.
If we get an ApiException at line #476, resource_version won't be assigned and referencing it will raise an exception
There was a problem hiding this comment.
Line 476 is called without a resource version, so it should never raises a 410 APIException. But for robustness, I've removed the resource_version reference from logging.
| @staticmethod | ||
| def _new_component(name: str, application: str, component_name: str, | ||
| image_repo: Optional[str], source_url: Optional[str], revision: Optional[str]) -> dict: | ||
| obj = { |
There was a problem hiding this comment.
Consider storing this definition as a jinja template, instead of hardcoding it. This could make it easier to find and maintain
|
|
||
| # https://konflux.pages.redhat.com/docs/users/how-tos/configuring/overriding-compute-resources.html | ||
| # ose-installer-artifacts fails with OOM with default values, hence bumping memory limit | ||
| obj["spec"]["taskRunSpecs"] = [{ |
There was a problem hiding this comment.
Same as previous comment, I think storing these definitions in template files would be best. Also ok with a follow up PR
- Move code for Konflux interactions into separate class `KonfluxClient`. - Use LRU cache for certain APIs.
vfreex
force-pushed
the
konflux-client
branch
from
d7fb85b to
375505f
Compare
|
@locriandev Thanks for the review. I've addressed your review suggestions again except the one for moving application/component definition to jinja files, which can be done in separate PRs. |
|
@vfreex: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit cdd8b01
into
openshift-eng:main
KonfluxClient.