Add fedora ci checks using the shared repo#2990
Conversation
Signed-off-by: Cristian Le <git@lecris.dev>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Fedora CI check for shared tests, utilizing a dedicated repository. The changes correctly integrate the new payload generation logic within the DownstreamTestingFarmJobHelper. Feedback focuses on improving code maintainability by adding a docstring to the new method and suggesting the use of constants for hardcoded repository details. Additionally, it's recommended to address or externalize the inline TODO comment for better tracking.
| def _payload_shared(self, distro: str, compose: str) -> dict: | ||
| git_repo = "https://forge.fedoraproject.org/ci/shared-tests" | ||
| git_ref = "main" | ||
| # The shared plans will define their own provision steps, requiring | ||
| # compose to be turned off. | ||
| payload = self._get_tf_base_payload(distro, None) | ||
| payload["test"] = { | ||
| "tmt": { | ||
| "url": git_repo, | ||
| "ref": git_ref, | ||
| }, | ||
| } | ||
| return payload |
There was a problem hiding this comment.
The newly added method _payload_shared is not trivial and should include a docstring explaining its purpose, arguments, and return value. This should follow the Google-style as specified in the repository's style guide. This improves code readability and maintainability.
Repository Style Guide: lines 378, 410
References
- Non-trivial methods of models must include docstrings describing the operation and accurate type hints, especially for the returned value, as incorrect type specified can lead to complicated run time issues. (link)
- With exception of trivial cases, all code must contain accurate and sufficiently detailed docstrings, formatted with accordance with the PEP 257 standard and in Google-style. (link)
| git_repo = "https://forge.fedoraproject.org/ci/shared-tests" | ||
| git_ref = "main" |
There was a problem hiding this comment.
The git_repo and git_ref values are hardcoded strings. It would be more maintainable to define these as constants, especially if they might be reused or changed in the future. This makes the code easier to update and reduces the chance of errors if the values need to be modified.
| # TODO: this placeholder would no longer be needed if all the other tests | ||
| # are defined in it. It requires splitting the check status for each plan | ||
| # and restarting individual plans (either from this shared or custom) |
There was a problem hiding this comment.
This TODO comment indicates a future refactoring or improvement. It's good to acknowledge, but for better tracking and to keep the codebase clean, consider creating a dedicated issue in your issue tracker for this task. This ensures the work is formally tracked and doesn't get lost as the codebase evolves.
|
✔️ pre-commit SUCCESS in 1m 50s |
1 participant
TODO:
produntil we switch the sources of the tests)Related to #2914
RELEASE NOTES BEGIN
Added the plans from forge.fp.o/ci/shared-tests. These plans
are currently only run in the
stgdeployment, and additional tests will appear first gated by thestgdeployment.RELEASE NOTES END