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.

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

Fedora CI scratch builds #2650

Merged

Conversation

lbarcziova
Copy link
Member

Related to #2602

RELEASE NOTES BEGIN

N/A

RELEASE NOTES END

@lbarcziova lbarcziova requested a review from a team as a code owner November 21, 2024 13:37
Copy link
Contributor

packit_service/config.py Show resolved Hide resolved
packit_service/worker/handlers/abstract.py Outdated Show resolved Hide resolved
Comment on lines +15 to +16
class FedoraCIHelper:
status_name: str = "Packit - scratch build"
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand the need for a new helper.

The KojiBuildJobHelper has some more methods that could be useful when building koji builds that are missing in the generic FedoraCIHelper (if I am not wrong).
If we need the FedoraCIHelper because of the reporting (that is different in Pagure) then I am wondering if we could factor it out in the StatusReporter or somewhere else related with the reporting. And when creating the helpers (that we already have) we can choose which kind of reporter inject?

I just fear that we will need to duplicate all our helpers, creating a new "base".

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, at the moment, it really mostly wraps up the status reporting, in my initial implementation it contained some other logic common for the 2 new handlers. So that was the main idea, to have the common logic in here. Other thing is that I didn't want to adjust too much of existing code since this is more of a prototype and might be later changed/moved, therefore I wanted to keep it in one separate place. But having it as it is now, I am ok to just repeat the status reporting logic for the 2 handlers.

Copy link
Member

Choose a reason for hiding this comment

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

It does not need to be done now, but I agree with Maja that we should be cautious about adding more helpers. (TBH, I don't think this was a good idea when introducing them... yes, I am probably the one to blame..;)

Copy link
Member

@mfocko mfocko left a comment

Choose a reason for hiding this comment

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

LGTM 👀

packit_service/config.py Outdated Show resolved Hide resolved
packit_service/worker/handlers/abstract.py Outdated Show resolved Hide resolved
packit_service/worker/jobs.py Outdated Show resolved Hide resolved
packit_service/worker/handlers/koji.py Outdated Show resolved Hide resolved
Copy link
Contributor

Copy link
Contributor

For the prototype for packit#2602. This will be likely moved/adjusted later.
For now mostly for testing this ourselves, we decided to run the jobs
just for the projects defined in our service config.
No package configuration is needed.
So that it can be used for both upstream and downstream reporting.
Copy link
Contributor

@lbarcziova lbarcziova added the mergeit When set, zuul wil gate and merge the PR. label Nov 26, 2024
Copy link
Contributor

Build succeeded (gate pipeline).
https://softwarefactory-project.io/zuul/t/packit-service/buildset/1bb9ead4f4754dcabfaadde9a2a8835b

✔️ pre-commit SUCCESS in 2m 15s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit b06de6a into packit:main Nov 26, 2024
4 checks passed
@lbarcziova lbarcziova deleted the fedora-ci-scratch-builds branch November 26, 2024 12:42
Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

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

I know I am a bit late for the party but I like the implementation and don't have any concerns.. Thanks, Laura! I am really looking forward to all this!

@@ -104,6 +107,30 @@ def _add_to_mapping(kls: type["JobHandler"]):
return _add_to_mapping


def reacts_to_as_fedora_ci(event: type["Event"]):
Copy link
Member

Choose a reason for hiding this comment

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

oo, decorators!

Comment on lines +15 to +16
class FedoraCIHelper:
status_name: str = "Packit - scratch build"
Copy link
Member

Choose a reason for hiding this comment

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

It does not need to be done now, but I agree with Maja that we should be cautious about adding more helpers. (TBH, I don't think this was a good idea when introducing them... yes, I am probably the one to blame..;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergeit When set, zuul wil gate and merge the PR.
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

5 participants