-
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
Consider identifiers when retriggering #2658
Changes from all commits
01a99c9
618224f
47dc64e
474b649
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
"""Add identifier to copr build | ||
|
||
Revision ID: d625d6c1122f | ||
Revises: f69687c314c5 | ||
Create Date: 2024-11-22 13:05:49.763917 | ||
|
||
""" | ||
|
||
import sqlalchemy as sa | ||
|
||
from alembic import op | ||
|
||
# revision identifiers, used by Alembic. | ||
revision = "d625d6c1122f" | ||
down_revision = "f69687c314c5" | ||
branch_labels = None | ||
depends_on = None | ||
|
||
|
||
def upgrade(): | ||
# ### commands auto generated by Alembic - please adjust! ### | ||
op.add_column( | ||
"copr_build_targets", sa.Column("identifier", sa.String(), default="", nullable=True) | ||
) | ||
# ### end Alembic commands ### | ||
|
||
|
||
def downgrade(): | ||
# ### commands auto generated by Alembic - please adjust! ### | ||
op.drop_column("copr_build_targets", "identifier") | ||
# ### end Alembic commands ### |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,8 +53,8 @@ def __init__( | |
metadata: EventData, | ||
db_project_event: ProjectEventModel, | ||
job_config: JobConfig, | ||
build_targets_override: Optional[set[str]] = None, | ||
tests_targets_override: Optional[set[str]] = None, | ||
build_targets_override: Optional[set[tuple[str, str]]] = None, | ||
tests_targets_override: Optional[set[tuple[str, str]]] = None, | ||
pushgateway: Optional[Pushgateway] = None, | ||
): | ||
super().__init__( | ||
|
@@ -67,8 +67,8 @@ def __init__( | |
pushgateway=pushgateway, | ||
) | ||
self.run_model: Optional[PipelineModel] = None | ||
self.build_targets_override: Optional[set[str]] = build_targets_override | ||
self.tests_targets_override: Optional[set[str]] = tests_targets_override | ||
self.build_targets_override: Optional[set[tuple[str, str]]] = build_targets_override | ||
self.tests_targets_override: Optional[set[tuple[str, str]]] = tests_targets_override | ||
self.pushgateway = pushgateway | ||
|
||
# lazy properties | ||
|
@@ -199,7 +199,7 @@ def build_targets(self) -> set[str]: | |
""" | ||
if self.build_targets_override: | ||
logger.debug(f"Build targets override: {self.build_targets_override}") | ||
return self.build_targets_all & self.build_targets_override | ||
return self.build_targets_all & {target for target, _ in self.build_targets_override} | ||
|
||
return self.build_targets_all | ||
|
||
|
@@ -270,13 +270,24 @@ def build_targets_for_test_job(self, test_job_config: JobConfig) -> set[str]: | |
|
||
if self.build_targets_override: | ||
logger.debug(f"Build targets override: {self.build_targets_override}") | ||
targets_override.update(self.build_targets_override) | ||
targets_override.update( | ||
[ | ||
target | ||
for (target, identifier) in self.build_targets_override | ||
if identifier == (test_job_config.identifier or "") | ||
] | ||
) | ||
|
||
if self.tests_targets_override: | ||
logger.debug(f"Test targets override: {self.tests_targets_override}") | ||
targets_override.update( | ||
self.test_target2build_target_for_test_job(target, test_job_config) | ||
for target in self.tests_targets_override | ||
self.test_target2build_target_for_test_job(t, test_job_config) | ||
for t in [ | ||
target | ||
for (target, identifier) in self.tests_targets_override | ||
if identifier | ||
== (test_job_config.identifier if test_job_config.identifier else "") | ||
] | ||
) | ||
|
||
return configured_targets & targets_override if targets_override else configured_targets | ||
|
@@ -314,14 +325,22 @@ def tests_targets_for_test_job(self, test_job_config: JobConfig) -> set[str]: | |
|
||
if self.build_targets_override: | ||
logger.debug(f"Build targets override: {self.build_targets_override}") | ||
for target in self.build_targets_override: | ||
targets_override.update( | ||
self.build_target2test_targets_for_test_job(target, test_job_config), | ||
) | ||
for target, identifier in self.build_targets_override: | ||
if identifier == (test_job_config.identifier if test_job_config.identifier else ""): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need the ternary there? 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
targets_override.update( | ||
self.build_target2test_targets_for_test_job(target, test_job_config), | ||
) | ||
|
||
if self.tests_targets_override: | ||
logger.debug(f"Test targets override: {self.tests_targets_override}") | ||
targets_override.update(self.tests_targets_override) | ||
targets_override.update( | ||
[ | ||
target | ||
for target, identifier in self.tests_targets_override | ||
if identifier | ||
== (test_job_config.identifier if test_job_config.identifier else "") | ||
] | ||
) | ||
|
||
return ( | ||
configured_targets & targets_override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,8 +80,8 @@ def __init__( | |
metadata: EventData, | ||
db_project_event: ProjectEventModel, | ||
job_config: JobConfig, | ||
build_targets_override: Optional[set[str]] = None, | ||
tests_targets_override: Optional[set[str]] = None, | ||
build_targets_override: Optional[set[tuple[str, str]]] = None, | ||
tests_targets_override: Optional[set[tuple[str, str]]] = None, | ||
Comment on lines
+83
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a feeling you're changing the meaning of this attribute, so the name no longer holds very well, but… I guess that's a different rabbit hole to go down… There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say they are still targets override but a bit more specific. They are targets for a specific identifier. The most efficient structure here is probably a dictionary and not a |
||
pushgateway: Optional[Pushgateway] = None, | ||
celery_task: Optional[CeleryTask] = None, | ||
copr_build_group_id: Optional[int] = None, | ||
|
@@ -605,6 +605,7 @@ def _get_or_create_build_group(self) -> CoprBuildGroupModel: | |
status=BuildStatus.waiting_for_srpm, | ||
copr_build_group=group, | ||
task_accepted_time=self.metadata.task_accepted_time, | ||
identifier=self.job_config.identifier, | ||
) | ||
|
||
if unprocessed_chroots: | ||
|
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.
just to be sure, I also remember some discussions about the empty set vs None for *targets_override, is it ok to have empty set here instead of None?
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.
I think it is, however I want to test this a bit deeper in the staging instance.