-
Notifications
You must be signed in to change notification settings - Fork 49
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
Implement OpenScanHub prototype #2472
Implement OpenScanHub prototype #2472
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
e6c6ab7
to
1d4d3b6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Build succeeded. ✔️ pre-commit SUCCESS in 2m 17s |
Build succeeded. ✔️ pre-commit SUCCESS in 2m 14s |
os.getenv("PACKIT_USER_AGENT") | ||
or f"packit-service/{ps_version} ([email protected])" | ||
) | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be you can retry on timeouts. This can go as a TODO note.
we have discussed with @siteshwar that we will see whether getting this on staging will uncover some potential problems with OpenScanHub (same as getting it on prod that will cause higher load on OpenScanHub than usual) and could disable the config option depending on that if there will be any issues. |
Wouldn't it be better to move the new methods from |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I assumed this will eventually be moved to a separate handler class once the config is refined later, but as you say, it makes sense to do it already, will change it. |
@@ -174,6 +174,35 @@ def job_project(self) -> Optional[str]: | |||
|
|||
return self.default_project_name | |||
|
|||
def job_project_for_commit_job_config(self, job_config) -> Optional[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we do this somewhere else already? 👀
This comment was marked as outdated.
This comment was marked as outdated.
Build succeeded. ✔️ pre-commit SUCCESS in 2m 30s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. All suggestions are just nitpicks, feel free to ignore them.
Build succeeded. ✔️ pre-commit SUCCESS in 2m 28s |
Build succeeded. ✔️ pre-commit SUCCESS in 2m 10s |
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 2m 15s |
8de1710
into
packit:main
Openscanhub prototype docs Related to #899 Related to packit/packit-service#2472 Reviewed-by: Nikola Forró
Remove trailing dot from OSH task comments ... to make it easier to copy dashboard links. TODO: Write new tests or update the old ones to cover new functionality. Update doc-strings where appropriate. Update or write new documentation in packit/packit.dev. ‹fill in› Fixes Related to #2472 Merge before/after RELEASE NOTES BEGIN RELEASE NOTES END Reviewed-by: Laura Barcziová
Fixes #2454
Fixes #2463
TODO:
RELEASE NOTES BEGIN
We have added the initial version of functionality for running scans in OpenScanHub. You can read more about this functionality
here.
RELEASE NOTES END