Add more customizable details to failure comments #2245

majamassarini · 2023-11-03T09:36:59Z

Fixes #2196
Merge before packit/packit.dev#769

RELEASE NOTES BEGIN

User can now further customize comment messages (written by Packit as a reaction of a failed job). The user can include in the customized message the following new placeholders:

logs_url: service's logs url; Copr, Koji or Testing Farm service depending on the Packit job
packit_dashboard_url: Packit dashboard url for the job
external_dashboard_url: service dashboard url; Copr, Koji or Testing Farm service depending on the Packit job

RELEASE NOTES END

softwarefactory-project-zuul · 2023-11-03T09:45:13Z

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/c01161773784471c869148ad3622b916

✔️ pre-commit SUCCESS in 1m 57s
✔️ packit-service-tests SUCCESS in 2m 02s
❌ packit-service-tests-openshift RETRY_LIMIT in 7m 42s

lbarcziova

nice job! Only one question regarding the variable naming

lbarcziova · 2023-11-03T13:44:07Z

packit_service/constants.py

+    "copr_build_packit_info",
+    "srpm_build_packit_info",
+    "copr_build_logs",
+    "koji_build_packit_info",
+    "koji_build_logs",
+    "testing_farm_packit_info",
+    "testing_farm_logs",


I am wondering if we could consolidate these only to something like packit_dashboard_url and logs_url 🤔 e.g. when someone configures a Copr build job, it can fail either in the SRPM phase or Copr build itself and it would be probably easier for user to configure only something like

One of the Copr builds failed for commit {commit_sha}, ping @admin, packit dashboard: {packit_dashboard_url}, logs: {logs_url}

I am on the fence here if we should support both specific and generic ones or just the generic ones.

What do you mean for the generic ones 😅?

What do you mean for the generic ones 😅?

Sorry, I didn't know how to call the ones Laura suggested -- like logs_url (unlike the "specific" ones like copr_build_url)...

Thinking more about it I agree with Laura, the user can't really use in a message logs_url coming from different services or a packit_dashboard_url for a different job... For this reason I think we don't need to differentiate them and we can make life easier for the user having only two placeholders: packit_dashboard_url and logs_url.

👍 Makes sense -- and we can easily add more if people ask for that.

~~Maybe, I would provide and differentiate also (not sure about the names) web_url and logs_url. (We have both for Copr -- on links to Copr frontend view, one to log.)~~

edit: OK, Just seen your comment bellow...

packit_service/constants.py

lachmanfrantisek · 2023-11-03T14:27:22Z

packit_service/constants.py

+    "copr_build_packit_info",
+    "srpm_build_packit_info",
+    "copr_build_logs",
+    "koji_build_packit_info",
+    "koji_build_logs",
+    "testing_farm_packit_info",
+    "testing_farm_logs",


What about using a url suffix for all the links?

lachmanfrantisek · 2023-11-03T15:04:52Z

packit_service/worker/helpers/build/build_helper.py

+            # every detail (the user would like to have)
+            # could be missing in the calling job
+            # iterate over all the details until we can build the user string


Since you have a name of all the keys, can't we specify all and the unknown to None/"unknown"?

But I like you are trying to resolve this situation.

Also, it would be nice to cover this in the tests...

I think I am not following you. There exist a test for the situation, I wrote the code using the test. Or I don't understand which situation do you mean 😅.
Regarding the first hint, I thought that a message like copr build logs {no entry for copr_build_packit_info} can be more understandable for the user than copr build logs None/unknown but maybe I did not understand your suggestion?

I mixed multiple points here...;)

Tests:

My bad, I missed that test-case. Sorry.

Wording:

I agree {no entry for copr_build_packit_info} is better than None/unknown. => I like the UX.

Code:

That was my main point if we can't do this in a "less technical" way. (Or, how to describe it.) Since we have the list of all the possible variables defined in constants.py, we can always provide all the variables to the formatted (with no entry ... as default) and avoid catching of the KeyError.

Thanks, now I understand and I agree.

lachmanfrantisek · 2023-11-03T15:09:31Z

packit_service/constants.py

+    "commit_sha",
+    "copr_build_packit_info",
+    "srpm_build_packit_info",
+    "copr_build_logs",


With Copr, we differentiate logs URL and web URL. This would be nice to cover as well.

If I go with the Laura's suggestion, then I can think to have 3 links:

packit_dashboard_url
logs_url
external_dashboard_url -> the copr web url can be linked here? And maybe also the koji dashboard build detail, that I think we have?

Yes, something like that would be nice.

softwarefactory-project-zuul · 2023-11-06T08:47:11Z

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/36b622320b39435eac413a7eaf5285f0

❌ pre-commit FAILURE in 1m 49s
❌ packit-service-tests FAILURE in 1m 37s
❌ packit-service-tests-openshift RETRY_LIMIT in 7m 29s

softwarefactory-project-zuul · 2023-11-06T12:30:52Z

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/87be26d9faa44d4583cec11e171d403d

✔️ pre-commit SUCCESS in 25m 52s
✔️ packit-service-tests SUCCESS in 2m 06s
❌ packit-service-tests-openshift RETRY_LIMIT in 7m 26s

lachmanfrantisek

Nice! Just one small suggestion.

lachmanfrantisek · 2023-11-06T12:40:55Z

packit_service/constants.py

+    "commit_sha": "{no entry for commit_sha}",
+    "packit_dashboard_url": "{no entry for packit_dashboard_url}",
+    "external_dashboard_url": "{no entry for external_dashboard_url}",
+    "logs_url": "{no entry for logs_url}",


Can't we generate the failure message automatically if it's just {no entry for $VALUE}?

lachmanfrantisek · 2023-11-06T12:41:52Z

packit_service/worker/helpers/build/build_helper.py

+        all_kwargs = copy.copy(FAILURE_COMMENT_MESSAGE_VARIABLES)
+        all_kwargs["commit_sha"] = self.db_project_event.commit_sha
+        all_kwargs.update(kwargs)
+        formatted_message = configured_message.format(**all_kwargs)


Co-authored-by: Laura Barcziová <[email protected]>

softwarefactory-project-zuul · 2023-11-06T13:15:16Z

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/a747af8370824aaf8454a20f8bb73038

✔️ pre-commit SUCCESS in 1m 52s
✔️ packit-service-tests SUCCESS in 2m 04s
❌ packit-service-tests-openshift RETRY_LIMIT in 7m 28s

lbarcziova

nicely done!

Add more customizable details for comment failures Merge after packit/packit-service#2245 Reviewed-by: František Lachman <[email protected]> Reviewed-by: Laura Barcziová Reviewed-by: Maja Massarini

majamassarini mentioned this pull request Nov 3, 2023

Add more customizable details for comment failures packit/packit.dev#769

Merged

lbarcziova reviewed Nov 3, 2023

View reviewed changes

lachmanfrantisek reviewed Nov 3, 2023

View reviewed changes

majamassarini force-pushed the fix/packit-service/2196 branch from b6e7faf to 15e0ad1 Compare November 6, 2023 12:03

lachmanfrantisek approved these changes Nov 6, 2023

View reviewed changes

Add more customizable details to failure comments

d7ea2de

Co-authored-by: Laura Barcziová <[email protected]>

majamassarini force-pushed the fix/packit-service/2196 branch from 15e0ad1 to d7ea2de Compare November 6, 2023 13:07

majamassarini merged commit 188ae23 into packit:main Nov 6, 2023

lbarcziova reviewed Nov 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more customizable details to failure comments #2245

Add more customizable details to failure comments #2245

majamassarini commented Nov 3, 2023 •

edited

Loading

softwarefactory-project-zuul bot commented Nov 3, 2023

lbarcziova left a comment

lbarcziova Nov 3, 2023

lachmanfrantisek Nov 3, 2023 •

edited

Loading

majamassarini Nov 6, 2023

lachmanfrantisek Nov 6, 2023

majamassarini Nov 6, 2023

lachmanfrantisek Nov 6, 2023

lachmanfrantisek Nov 6, 2023 •

edited

Loading

lachmanfrantisek Nov 3, 2023

lachmanfrantisek Nov 3, 2023

lachmanfrantisek Nov 3, 2023

majamassarini Nov 6, 2023

lachmanfrantisek Nov 6, 2023

majamassarini Nov 6, 2023

lachmanfrantisek Nov 3, 2023

majamassarini Nov 6, 2023

lachmanfrantisek Nov 6, 2023

softwarefactory-project-zuul bot commented Nov 6, 2023

softwarefactory-project-zuul bot commented Nov 6, 2023

lachmanfrantisek left a comment

lachmanfrantisek Nov 6, 2023

lachmanfrantisek Nov 6, 2023

softwarefactory-project-zuul bot commented Nov 6, 2023

lbarcziova left a comment

Add more customizable details to failure comments #2245

Add more customizable details to failure comments #2245

Conversation

majamassarini commented Nov 3, 2023 • edited Loading

softwarefactory-project-zuul bot commented Nov 3, 2023

lbarcziova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lachmanfrantisek Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lachmanfrantisek Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Nov 6, 2023

softwarefactory-project-zuul bot commented Nov 6, 2023

lachmanfrantisek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Nov 6, 2023

lbarcziova left a comment

Choose a reason for hiding this comment

majamassarini commented Nov 3, 2023 •

edited

Loading

lachmanfrantisek Nov 3, 2023 •

edited

Loading

lachmanfrantisek Nov 6, 2023 •

edited

Loading