Consider identifiers when retriggering #2658
Conversation
|
Build failed. ❌ pre-commit FAILURE in 2m 23s |
packit_service/models.py
Outdated
| @@ -2102,6 +2104,8 @@ class CoprBuildTargetModel(GroupAndTargetModelConnector, Base): | |||
|
|
|||
| scan = relationship("OSHScanModel", back_populates="copr_build_target") | |||
|
|
|||
| identifier = Column(String, default="") | |||
There was a problem hiding this comment.
do we need the default? Just checking for TF targets we don't define this, so I would try to stay consistent
There was a problem hiding this comment.
Ok is not a big issue not having the default here, I am always creating a new entry with the empty string as a default. You can read below why I think a comparison with an empty string more safe than a comparison with None
| return self.is_comment_event() and self.comment_arguments.packit_command == "test" | ||
| return self.is_comment_event() and self.comment_arguments.packit_command in ( | ||
| "test", | ||
| "retest-failed", |
| build_targets_override: Optional[set[tuple[str, str]]] = None, | ||
| tests_targets_override: Optional[set[tuple[str, str]]] = None, |
There was a problem hiding this comment.
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.
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 set anymore, however I would start with this simpler enhancement.
| 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.
Why do you need the ternary there? 👀
There was a problem hiding this comment.
job_config.identifier could be None, so yes I think we need it. A non specified identifier is the default identifier "".
We can also say that the default identifier is always None but in this case I am a bit afraid of the serialization, if we go through a serialization/deserialization process of a None string value I think we will end up with an empty str ""?
So I feel this comparison more safe.
majamassarini
force-pushed
the
fix/packit-service/1886
branch
from
4686a82 to
097a3fc
Compare
|
Build failed. ✔️ pre-commit SUCCESS in 2m 03s |
majamassarini
force-pushed
the
fix/packit-service/1886
branch
from
80e805e to
019e2fe
Compare
majamassarini
force-pushed
the
fix/packit-service/1886
branch
from
019e2fe to
999a064
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 00s |
| build_targets_override = ( | ||
| {(target, identifier_) for [target, identifier_] in event.get("build_targets_override")} | ||
| if event.get("build_targets_override") | ||
| else set() |
There was a problem hiding this comment.
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.
I think it is, however I want to test this a bit deeper in the staging instance.
Co-authored-by: Laura Barcziová <[email protected]>
Use duck typing check instead of isinstance. Inside the test I can't instantiate a proper type object with all the needed mocked properties.
majamassarini
force-pushed
the
fix/packit-service/1886
branch
from
a9dfaec to
474b649
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 02s |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 2m 06s |
softwarefactory-project-zuul
bot
merged commit fc33a6f
into
packit:main
Related to #1886