Move installing opm binaries to iib-worker Dockerfile#1300
Open
lipoja wants to merge 1 commit intorelease-engineering:mainfrom
Open
Move installing opm binaries to iib-worker Dockerfile#1300lipoja wants to merge 1 commit intorelease-engineering:mainfrom
lipoja wants to merge 1 commit intorelease-engineering:mainfrom
Conversation
Reviewer's GuideIntroduce a central iib_ocp_opm_mapping in the main Config, adjust the development config to rely on base formatting, and change failure cleanup behavior in general tasks to reset Docker configuration instead of running the broader build cleanup, in preparation for moving binaries into the iib image (with additional changes in the workers Dockerfile). Sequence diagram for updated failed_request_callback error handlingsequenceDiagram
participant CeleryWorker
participant failed_request_callback
participant reset_docker_config
participant set_request_state
CeleryWorker ->> failed_request_callback: on_task_failure(request, exc, task_id, args, kwargs, einfo)
alt exc is IIBError
failed_request_callback ->> failed_request_callback: msg = str(exc)
else exc is FinalStateOverwriteError
failed_request_callback ->> failed_request_callback: log.info("Request is in final state")
failed_request_callback ->> reset_docker_config: reset_docker_config()
failed_request_callback -->> CeleryWorker: return
else exc is other Exception
failed_request_callback ->> failed_request_callback: msg = "An unknown error occurred. See logs for details"
failed_request_callback ->> failed_request_callback: log.error(msg, exc_info=exc)
end
failed_request_callback ->> reset_docker_config: reset_docker_config()
failed_request_callback ->> set_request_state: set_request_state(request_id, failed, msg)
set_request_state -->> failed_request_callback: state updated
failed_request_callback -->> CeleryWorker: return
Class diagram for updated Config and DevelopmentConfigclassDiagram
class Config {
+Dict~str, Tuple~int, int~~ iib_binary_ranges
+Dict~str, str~ iib_ocp_opm_mapping
+str iib_opm_pprof_lock_required_min_version
+str iib_image_push_template
}
class DevelopmentConfig {
+str iib_registry
+Optional~str~ iib_request_logs_dir
+Optional~str~ iib_request_related_bundles_dir
+Optional~str~ iib_request_recursive_related_bundles_dir
+str iib_dogpile_backend
+dict iib_ocp_opm_mapping
}
DevelopmentConfig --|> Config
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by QodoMove binaries to IIB worker Dockerfile with verification
WalkthroughsDescription• Move OPM binaries to worker Dockerfile with SHA256 verification • Add OCP-OPM version mapping as default configuration • Replace _cleanup() with reset_docker_config() in containerized IIB • Install multiple OPM versions and operator-sdk with checksums Diagramflowchart LR
A["Dockerfile-workers"] -->|"Install OPM variants<br/>with SHA256 checks"| B["Multiple OPM versions<br/>in /usr/local/bin"]
C["config.py"] -->|"Add default mapping"| D["iib_ocp_opm_mapping<br/>configuration"]
E["general.py"] -->|"Replace cleanup call"| F["reset_docker_config<br/>for containerized IIB"]
B --> G["Default OPM symlink"]
File Changes1. docker/Dockerfile-workers
|
Code Review by Qodo
1. Dockerfile command chaining bug
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Now that
Configdefines a globaliib_ocp_opm_mapping, consider whether the override inDevelopmentConfigis still needed or if it should be removed/updated to avoid diverging mappings and confusion about the single source of truth. - Replacing
_cleanup()withreset_docker_config()infailed_request_callbacknarrows the cleanup behavior; if_cleanup()previously handled additional teardown beyond Docker config, consider whether that logic still needs to be invoked on failure in some way.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `Config` defines a global `iib_ocp_opm_mapping`, consider whether the override in `DevelopmentConfig` is still needed or if it should be removed/updated to avoid diverging mappings and confusion about the single source of truth.
- Replacing `_cleanup()` with `reset_docker_config()` in `failed_request_callback` narrows the cleanup behavior; if `_cleanup()` previously handled additional teardown beyond Docker config, consider whether that logic still needs to be invoked on failure in some way.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lipoja
force-pushed
the
move_binaries_to_iib
branch
from
051ff0a to
3955ecc
Compare
lipoja
changed the title
… default in config.py [CLOUDDST-31191]
lipoja
force-pushed
the
move_binaries_to_iib
branch
from
3955ecc to
4aac09e
Compare
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+49
to
+50
| ln -sfn /usr/local/bin/opm-v1.26.4 /usr/local/bin/opm \ | ||
| install_binary "operator-sdk" "https://github.com/operator-framework/operator-sdk/releases/download/v1.15.0/operator-sdk_linux_amd64" "d2065f1f7a0d03643ad71e396776dac0ee809ef33195e0f542773b377bab1b2a" |
There was a problem hiding this comment.
1. Dockerfile command chaining bug 🐞 Bug ✓ Correctness
The Dockerfile RUN block is missing a command separator between ln -sfn ... and `install_binary ..., so the shell will treat install_binary as extra args to ln` (or otherwise fail), preventing proper installation of the default opm symlink and operator-sdk. This can break runtime because the code expects a default opm binary name to exist.
Agent Prompt
### Issue description
`docker/Dockerfile-workers` has a malformed multi-command `RUN` step: `ln -sfn ...` is not terminated before calling `install_binary ...`, which can break the build and/or prevent creating the default `opm` symlink and installing `operator-sdk`.
### Issue Context
The worker code expects a default `opm` binary name (`iib_default_opm = 'opm'`). If the symlink isn’t created, tasks may fail when they fall back to the default opm.
### Fix Focus Areas
- docker/Dockerfile-workers[49-50]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
iib/workers/tasks/general.py
Outdated
Comment on lines
10
to
11
| from iib.workers.tasks.utils import request_logger, reset_docker_config | ||
| from iib.workers.tasks.build import _cleanup |
There was a problem hiding this comment.
2. Unused _cleanup import 🐞 Bug ⛯ Reliability
iib/workers/tasks/general.py still imports _cleanup but no longer uses it after switching the failure callback to reset_docker_config(). This should fail mandatory flake8 with F401 (unused import).
Agent Prompt
### Issue description
`iib/workers/tasks/general.py` imports `_cleanup` but no longer calls it, which will fail flake8 (unused import).
### Issue Context
flake8 is a mandatory tox environment and `.flake8` does not ignore F401 for this file.
### Fix Focus Areas
- iib/workers/tasks/general.py[7-12]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Update worker configuration and failure handling for OPM version mapping and Docker cleanup behavior.
New Features:
Enhancements: