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.

Sign up for GitHub

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

Jump to bottom

feat(reporter): Add id of current ORT run as label to ORT results #200

Merged
merged 1 commit into from
May 22, 2024
Merged

feat(reporter): Add id of current ORT run as label to ORT results #200

merged 1 commit into from
May 22, 2024

Conversation

wkl3nk
Copy link
Contributor

@wkl3nk wkl3nk commented May 7, 2024

Add the id of the current ORT run as label to the ORT result in order to have it available during report and notification generation.

@wkl3nk wkl3nk force-pushed the add-run-id-to-ort-result-labels branch from fdadf43 to 8fee6df Compare May 7, 2024 19:32
@wkl3nk wkl3nk force-pushed the add-run-id-to-ort-result-labels branch 2 times, most recently from b0e25c8 to b4cd334 Compare May 8, 2024 13:29
oheger-bosch
oheger-bosch reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@oheger-bosch oheger-bosch left a comment

Choose a reason for hiding this comment

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

As a general note: I would recommend to do the introduction of the new result generator class/function in a separate commit. Currently, the single commit does a lot of things.

workers/common/src/main/kotlin/common/OrtBaseResultGenerator.kt Outdated Show resolved Hide resolved
workers/common/src/main/kotlin/common/OrtBaseResultGenerator.kt Outdated Show resolved Hide resolved
@wkl3nk wkl3nk force-pushed the add-run-id-to-ort-result-labels branch 2 times, most recently from e41b09a to d722926 Compare May 14, 2024 12:48
@wkl3nk wkl3nk requested a review from oheger-bosch May 14, 2024 14:10
@wkl3nk wkl3nk force-pushed the add-run-id-to-ort-result-labels branch from d722926 to 0728582 Compare May 15, 2024 06:30
@wkl3nk wkl3nk requested a review from oheger-bosch May 15, 2024 06:44
sschuberth
sschuberth reviewed May 21, 2024
View reviewed changes
workers/common/src/main/kotlin/common/OrtRunService.kt
@@ -88,6 +89,10 @@ class OrtRunService(
private val scannerJobRepository: ScannerJobRepository,
private val scannerRunRepository: ScannerRunRepository
) {
companion object {
private const val RUN_ID_LABEL: String = "runId"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This should drop the explicit type.

@wkl3nk wkl3nk force-pushed the add-run-id-to-ort-result-labels branch from 0728582 to 07834ef Compare May 21, 2024 13:36
@wkl3nk
feat(reporter): Add id of current ORT run as label to ORT results
59a27f1 
Add the id of the current ORT run as label to the ORT result
in order to have it available during report and notification
generation.

Signed-off-by: Wolfgang Klenk <[email protected]>
@wkl3nk wkl3nk force-pushed the add-run-id-to-ort-result-labels branch from 07834ef to 59a27f1 Compare May 21, 2024 15:18
@oheger-bosch oheger-bosch added this pull request to the merge queue May 22, 2024
Merged via the queue into eclipse-apoapsis:main with commit 2f1f1e3 May 22, 2024
11 of 12 checks passed
@oheger-bosch oheger-bosch deleted the add-run-id-to-ort-result-labels branch May 22, 2024 05:26
@mnonnenmacher
Copy link
Contributor

mnonnenmacher commented May 22, 2024 •
edited
Loading

@oheger-bosch Please pay extra attention to the ECA check before adding PRs to the merge queue, we currently cannot make it required (https://gitlab.eclipse.org/eclipsefdn/it/api/git-eca-rest-api/-/issues/161, eclipse-apoapsis/.eclipsefdn#4).

@wkl3nk Please do not edit commits with the GitHub UI as this makes the ECA check fail: https://api.eclipse.org/git/eca/status/gh/eclipse-apoapsis/ort-server/200

@oheger-bosch
Copy link
Contributor

@oheger-bosch Please pay extra attention to the ECA check before adding PRs to the merge queue, we currently cannot make it required (https://gitlab.eclipse.org/eclipsefdn/it/api/git-eca-rest-api/-/issues/161, eclipse-apoapsis/.eclipsefdn#4).

@wkl3nk Please do not edit commits with the GitHub UI as this makes the ECA check fail: https://api.eclipse.org/git/eca/status/gh/eclipse-apoapsis/ort-server/200

What does this mean? Does the commit have to be dropped again?

@mnonnenmacher
Copy link
Contributor

@oheger-bosch Please pay extra attention to the ECA check before adding PRs to the merge queue, we currently cannot make it required (https://gitlab.eclipse.org/eclipsefdn/it/api/git-eca-rest-api/-/issues/161, eclipse-apoapsis/.eclipsefdn#4).
@wkl3nk Please do not edit commits with the GitHub UI as this makes the ECA check fail: https://api.eclipse.org/git/eca/status/gh/eclipse-apoapsis/ort-server/200

What does this mean? Does the commit have to be dropped again?

No, I think as we know the commit was authered by @wkl3nk and only the wrong email was used it should be fine.
However, looking at the git log the committer was overwritten anyway by [email protected] as you added the commit to the merge queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sschuberth sschuberth sschuberth left review comments

@oheger-bosch oheger-bosch oheger-bosch approved these changes

@bs-ondem bs-ondem Awaiting requested review from bs-ondem bs-ondem is a code owner

@MarcelBochtler MarcelBochtler Awaiting requested review from MarcelBochtler MarcelBochtler is a code owner

@mnonnenmacher mnonnenmacher Awaiting requested review from mnonnenmacher mnonnenmacher is a code owner

@nnobelis nnobelis Awaiting requested review from nnobelis nnobelis is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wkl3nk @mnonnenmacher @oheger-bosch @sschuberth