Implement triggering TF with build from other PR by lbarcziova · Pull Request #1658 · packit/packit-service

lbarcziova · 2022-09-15T12:44:57Z

TODO:

Polish the code.
Fix openshift tests.
Improve doc-strings.
Update or write new documentation in packit/packit.dev (Add docs about running TF with builds from other PR packit.dev#528).
Update view of the TF results in dashboard (I will open a dashboard PR once this is in staging)

Since this is a bigger change, maybe we can try the pair-review if anyone would be interested :D

RELEASE NOTES BEGIN
We have added support for running the tests with Copr builds built by Packit in another pull request (in a different repository). You can read more about this feature in our documentation.
RELEASE NOTES END

softwarefactory-project-zuul · 2022-09-19T07:52:17Z

Build succeeded.

✔️ pre-commit SUCCESS in 2m 50s
✔️ packit-service-tests SUCCESS in 2m 03s
✔️ packit-service-tests-openshift SUCCESS in 11m 51s

TomasTomecek

Very nice work with the refactoring!

I also like how straightforward this is.

packit_service/worker/helpers/testing_farm.py

softwarefactory-project-zuul · 2022-09-21T08:27:55Z

Build succeeded.

✔️ pre-commit SUCCESS in 2m 00s
✔️ packit-service-tests SUCCESS in 2m 06s
✔️ packit-service-tests-openshift SUCCESS in 11m 01s

mfocko

pls tell me I didn't spend 30 minutes on that second commit /o\

packit_service/models.py

mfocko · 2022-09-22T08:14:39Z

packit_service/models.py

+        Iterable["TFTTestRunTargetModel"],
+    ],
+    statuses_to_filter_with: List[str],
+) -> Optional[Set[Union["CoprBuildTargetModel", "TFTTestRunTargetModel"]]]:


Given the input, I guess you meant

Suggested change

) -> Optional[Set[Union["CoprBuildTargetModel", "TFTTestRunTargetModel"]]]:

) -> Optional[Union[Set["CoprBuildTargetModel"], Set["TFTTestRunTargetModel"]]]:

I haven't written this code, but yes, the types were wrong and not only here...I tried to correct it (1. commit), but I don't know how to explain some things to mypy (line 226 in models.py :D)..I would be glad for any advice

packit_service/models.py

packit_service/worker/helpers/testing_farm.py

softwarefactory-project-zuul · 2022-09-27T07:47:22Z

Build succeeded.

✔️ pre-commit SUCCESS in 1m 57s
✔️ packit-service-tests SUCCESS in 1m 58s
✔️ packit-service-tests-openshift SUCCESS in 11m 25s

Move methods of AbstractForgeIndependentEvent for filtering most recent copr build and testing farm target models by status to models.py as functions so that they can be used on other places.

softwarefactory-project-zuul · 2022-09-29T06:40:16Z

Build succeeded.

✔️ pre-commit SUCCESS in 2m 16s
✔️ packit-service-tests SUCCESS in 2m 08s
✔️ packit-service-tests-openshift SUCCESS in 11m 58s

Implement getting of additional Copr build target models that can be specified by test comment command PR argument, e.g. '/packit test repo/namespace#123'. In TestingFarmJobHandler pre-check check whether there are any successful recent Copr builds for that PR in the database and report to user. If TF is triggered by a comment with PR argument, include the artifact for the build from that PR, the packages from that build in the payload and add the TF run to the pipeline for that PR build in the database as well. Change the API for getting the TF runs to be able to get the additional build from the TF run. Also split the long methods in the TestingFarmJobHelper.

Create tests for the new usecase and update the existing tests to reflect the changes.

They both are quite independent, so they don't need to be in the abstract.py. This should avoid circular imports. Fixes packit#1618

softwarefactory-project-zuul · 2022-09-29T09:57:13Z

Build succeeded.

✔️ pre-commit SUCCESS in 2m 33s
✔️ packit-service-tests SUCCESS in 2m 35s
✔️ packit-service-tests-openshift SUCCESS in 13m 24s

softwarefactory-project-zuul · 2022-09-29T10:34:51Z

Build succeeded (gate pipeline).

✔️ pre-commit SUCCESS in 2m 04s

In packit/packit-service#1658 there was implemented that a TF run can be triggered with multiple Copr builds and API for TF runs changed (copr_build_id -> copr_build_ids) as well. This PR implements displaying those builds correctly in the TF result view.

lbarcziova · 2022-09-29T12:40:42Z

I have just tested it in lbarcziova/hello-world#1 and the ideal use case works just fine.

In packit/packit-service#1658 there was implemented that a TF run can be triggered with multiple Copr builds and API for TF runs changed (copr_build_id -> copr_build_ids) as well. This PR implements displaying those builds correctly in the TF result view.

Display multiple Copr builds for TF results if present In packit/packit-service#1658 there was implemented that a TF run can be triggered with multiple Copr builds and API for TF runs changed (copr_build_id -> copr_build_ids) as well. This PR implements displaying those builds correctly in the TF result view. Related to packit/packit-service#1658 The view for the TF runs with only one build stays the same and this is how it looks like for TF runs with more builds: RELEASE NOTES BEGIN N/A RELEASE NOTES END Reviewed-by: Matej Focko <None>

Add docs about running TF with builds from other PR Related to packit/packit-service#1658 Reviewed-by: Tomas Tomecek <tomas@tomecek.net> Reviewed-by: Laura Barcziová <None>

r0x0d · 2022-11-18T17:10:01Z

packit_service/worker/helpers/testing_farm.py

+        if not self._comment_command_parts and (
+            comment := self.metadata.event_dict.get("comment")
+        ):
+            self._comment_command_parts = get_packit_commands_from_comment(
+                comment,
+                packit_comment_command_prefix=self.service_config.comment_command_prefix,
+            )
+        return self._comment_command_parts


I know it's a bite late for this comment (and probably I'm not the most qualified person to suggest this), but this method looks a bit strange, specially if because there is a mutation happening to the self._comment_command_parts class-property, and we are returning the value we just assigned to a class-property that can be accessed later.

Ideally, I think it would be more readable and concise if this method doesn't return any result in it, but instead, override the value if necessary of the class-property and that's it.

My suggestion for this one would be removing the return statement at the end of the method and leaving the mutation where it is right now, especially because the class initializes the property as None.

Suggested change

if not self._comment_command_parts and (

comment := self.metadata.event_dict.get("comment")

):

self._comment_command_parts = get_packit_commands_from_comment(

comment,

packit_comment_command_prefix=self.service_config.comment_command_prefix,

)

return self._comment_command_parts

def comment_command_parts(self) -> Optional[List[str]]:

"""

List of packit comment command parts if the testing farm was triggered by a comment.

Note:

If the `self._comment_command_parts is not None, we will let the value as it is for now.

This method will only override the contents of the class-property if the same is `None`, and

we detect a comment from the `self.metadata`

Example:

'/packit test' -> ["test"]

'/packit test namespace/repo#pr' -> ["test", "namespace/repo#pr"]

"""

if not self._comment_command_parts and (

comment := self.metadata.event_dict.get("comment")

):

self._comment_command_parts = get_packit_commands_from_comment(

comment,

packit_comment_command_prefix=self.service_config.comment_command_prefix,

)

If you choose that, then you necessarily don't need to transform this method into a property, but rather, leave it as a method.

If you would still prefer to have it as a property because that would trigger a lot of changes in other parts of the code, then, I think something like this could also be good:

Suggested change

if not self._comment_command_parts and (

comment := self.metadata.event_dict.get("comment")

):

self._comment_command_parts = get_packit_commands_from_comment(

comment,

packit_comment_command_prefix=self.service_config.comment_command_prefix,

)

return self._comment_command_parts

def comment_command_parts(self) -> Optional[List[str]]:

"""

List of packit comment command parts if the testing farm was triggered by a comment.

Note:

If the `self._comment_command_parts is not None, we will let the value as it is for now.

This method will only override the contents of the class-property if the same is `None`, and

we detect a comment from the `self.metadata`

Example:

'/packit test' -> ["test"]

'/packit test namespace/repo#pr' -> ["test", "namespace/repo#pr"]

"""

if comment := self.metadata.event_dict.get("comment"):

return get_packit_commands_from_comment(

comment,

packit_comment_command_prefix=self.service_config.comment_command_prefix,

)

else:

return None

I'm recommending this especially because I can see that self._comment_command_parts: Optional[List[str]] = None is not used anywhere else other than here, so, probably, it would be easier to not have this class-property and have it either the method or the method-property instead.

But of course, again, I may not be the best person to be proposing this right now, especially because the PR is already merged. Feel free to just close this comment if you think it's not worth it.

P.S: In my opinion, I lean towards the second suggestion.

A typo was introduced in packit#1658 where the `namespace` and `repo` were in the wrong order. Also removed the parameter `url` as the default value for `url` in `report_status_to_tests` is already an empty string.

Fix a typo in the failure message A typo was introduced in #1658 where the namespace and repo were in the wrong order. Also removed the parameter url as the default value for url in report_status_to_tests is already an empty string. Reviewed-by: Laura Barcziová <None>

This comment was marked as outdated.

Sign in to view

lbarcziova force-pushed the tf-more-builds branch from f1bff4a to a6d64d5 Compare September 15, 2022 13:57

This comment was marked as outdated.

Sign in to view

lbarcziova force-pushed the tf-more-builds branch from a6d64d5 to 3958a09 Compare September 19, 2022 06:52

This comment was marked as outdated.

Sign in to view

lbarcziova force-pushed the tf-more-builds branch from 3958a09 to 94e0565 Compare September 19, 2022 07:39

lbarcziova marked this pull request as ready for review September 19, 2022 07:54

TomasTomecek reviewed Sep 19, 2022

View reviewed changes

packit_service/worker/helpers/testing_farm.py Show resolved Hide resolved

lbarcziova mentioned this pull request Sep 19, 2022

Add docs about running TF with builds from other PR packit/packit.dev#528

Merged

lbarcziova force-pushed the tf-more-builds branch from 94e0565 to cf1e743 Compare September 21, 2022 08:16

mfocko approved these changes Sep 22, 2022

View reviewed changes

lbarcziova force-pushed the tf-more-builds branch from cf1e743 to bd9d803 Compare September 27, 2022 07:35

Move methods for filtering models to models.py

d1e5cec

Move methods of AbstractForgeIndependentEvent for filtering most recent copr build and testing farm target models by status to models.py as functions so that they can be used on other places.

lbarcziova force-pushed the tf-more-builds branch from bd9d803 to 5cccddb Compare September 29, 2022 06:26

lbarcziova added 3 commits September 29, 2022 11:41

Update tests after implementing TF with more builds

f886172

Create tests for the new usecase and update the existing tests to reflect the changes.

Move get_packit_commands_from_comment and CeleryTask class

d7ddff9

They both are quite independent, so they don't need to be in the abstract.py. This should avoid circular imports. Fixes packit#1618

lbarcziova force-pushed the tf-more-builds branch from 5cccddb to d7ddff9 Compare September 29, 2022 09:42

lbarcziova added the mergeit Merge via Zuul label Sep 29, 2022

softwarefactory-project-zuul bot merged commit 31b879e into packit:main Sep 29, 2022

lbarcziova mentioned this pull request Sep 29, 2022

Display multiple Copr builds for TF results if present packit/dashboard#191

Merged

lbarcziova deleted the tf-more-builds branch September 29, 2022 12:54

r0x0d reviewed Nov 18, 2022

View reviewed changes

r0x0d mentioned this pull request Nov 18, 2022

Fix a typo in the failure message #1762

Merged

	) -> Optional[Set[Union["CoprBuildTargetModel", "TFTTestRunTargetModel"]]]:
	) -> Optional[Union[Set["CoprBuildTargetModel"], Set["TFTTestRunTargetModel"]]]:

-        if not self._comment_command_parts and (
-            comment := self.metadata.event_dict.get("comment")
-        ):
-            self._comment_command_parts = get_packit_commands_from_comment(
-                comment,
-                packit_comment_command_prefix=self.service_config.comment_command_prefix,
-            )
-        return self._comment_command_parts
+    def comment_command_parts(self) -> Optional[List[str]]:
+        """
+        List of packit comment command parts if the testing farm was triggered by a comment.
+        Note:
+            If the `self._comment_command_parts is not None, we will let the value as it is for now.
+            This method will only override the contents of the class-property if the same is `None`, and
+            we detect a comment from the `self.metadata`
+        Example:
+            '/packit test' -> ["test"]
+            '/packit test namespace/repo#pr' -> ["test", "namespace/repo#pr"]
+        """
+        if comment := self.metadata.event_dict.get("comment"):
+            return get_packit_commands_from_comment(
+                comment,
+                packit_comment_command_prefix=self.service_config.comment_command_prefix,
+            )
+       else:
+           return None

Conversation

lbarcziova commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

softwarefactory-project-zuul bot commented Sep 19, 2022

Uh oh!

TomasTomecek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

softwarefactory-project-zuul bot commented Sep 21, 2022

Uh oh!

mfocko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mfocko Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

lbarcziova Sep 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

softwarefactory-project-zuul bot commented Sep 27, 2022

Uh oh!

softwarefactory-project-zuul bot commented Sep 29, 2022

Uh oh!

softwarefactory-project-zuul bot commented Sep 29, 2022

Uh oh!

softwarefactory-project-zuul bot commented Sep 29, 2022

Uh oh!

lbarcziova commented Sep 29, 2022

Uh oh!

r0x0d Nov 18, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lbarcziova commented Sep 15, 2022 •

edited

Loading