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

Fix searching for base build when triggering OpenScanHub scan #2518

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

nforro
Copy link
Member

@nforro nforro commented Sep 6, 2024

Fixes #2495.

Two notes:

  • I'm not sure what are the performance implications of enabling require_git_repo_in_local_project
  • In case there are no successful builds but there are a lot of commits it could take a while before it fails, however I'm not sure how to detect such case

@nforro nforro requested a review from a team as a code owner September 6, 2024 08:37
Copy link
Contributor

@lbarcziova
Copy link
Member

I'm not sure what are the performance implications of enabling require_git_repo_in_local_project

it means now for the copr build handler tasks, the git repo would be cloned, which we tried to avoid previously :/ do you think we could solve this in another way?

@nforro
Copy link
Member Author

nforro commented Sep 6, 2024

it means now for the copr build handler tasks, the git repo would be cloned, which we tried to avoid previously :/ do you think we could solve this in another way?

I suppose it would be possible (though a bit hacky) to only enable require_git_repo_in_local_project per instance and rebuild LocalProject, would that make sense?

@lbarcziova
Copy link
Member

I am still not sure we should be cloning the whole repo to get few commit hashes, what about getting it via API?

@nforro
Copy link
Member Author

nforro commented Sep 6, 2024

I am still not sure we should be cloning the whole repo to get few commit hashes, what about getting it via API?

It seems possible for GitHub and GitLab, not for Pagure, but I suppose we don't care 🙂

@mfocko
Copy link
Member

mfocko commented Sep 6, 2024

Wouldn't it be easier to get last successful build? From the practical PoV I don't really see a difference between:

  • checking commit by commit for a build
  • taking last successful build

Or is the issue with different packages in the Copr repo? Can't we just filter by the package name in that case?

@nforro
Copy link
Member Author

nforro commented Sep 6, 2024

I think the problem is we need to get only builds triggered from the target branch of a PR. Although it would be weird to build in the same Copr project from different branches, it is possible.

@nforro
Copy link
Member Author

nforro commented Sep 6, 2024

I have opened packit/ogr#857.

Copy link
Contributor

@nforro
Copy link
Member Author

nforro commented Sep 9, 2024

I decided to leave target_branch_head_commit there and use it first, since it should work in most cases and get_commits() could take some time, so it makes sense to call it only if necessary.

@lachmanfrantisek
Copy link
Member

I think the problem is we need to get only builds triggered from the target branch of a PR. Although it would be weird to build in the same Copr project from different branches, it is possible.

We can use Build directories for this purpose. But this would require quite a lot of work.

@nforro
Copy link
Member Author

nforro commented Sep 9, 2024

We can use Build directories for this purpose. But this would require quite a lot of work.

Are you referring to this? I like the idea of having commit/release triggered builds in a project and PR builds in its subdirectories (I'm just starting to work on #2432), but I'm not sure if we should force users do configure that if they want to use OSH. Or maybe I just don't get your comment 🙂

Copy link
Member

@lbarcziova lbarcziova left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@nforro nforro added the mergeit When set, zuul wil gate and merge the PR. label Sep 12, 2024
Copy link
Contributor

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

✔️ pre-commit SUCCESS in 9m 19s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 8e54543 into packit:main Sep 12, 2024
4 checks passed
@nforro nforro deleted the osh_base_build branch September 12, 2024 07:22
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
None yet
Development

Successfully merging this pull request may close these issues.

Fix searching for base build when triggering OpenScanHub scan
4 participants