Pin the osbuild-composer commit for testing#535
Pin the osbuild-composer commit for testing#535achilleas-k wants to merge 6 commits intoosbuild:mainfrom
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Now that
osbuild-composeris pinned viaSchutzfile, consider removing or repurposing theget_last_passed_commitfunction if it's no longer used to avoid dead code and confusion about the current source of the commit. - It might be safer to handle missing or
null.dependencies.composer.commitentries inSchutzfileexplicitly (e.g., with a clear error message) so the deploy fails early and predictably when the pin is not defined. - For consistency and readability, consider using the same style of
jqinvocation for both composer and osbuild commits (either both withcator both directly on the file) and aligning the JSON paths if their structures are intended to be parallel.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `osbuild-composer` is pinned via `Schutzfile`, consider removing or repurposing the `get_last_passed_commit` function if it's no longer used to avoid dead code and confusion about the current source of the commit.
- It might be safer to handle missing or `null` `.dependencies.composer.commit` entries in `Schutzfile` explicitly (e.g., with a clear error message) so the deploy fails early and predictably when the pin is not defined.
- For consistency and readability, consider using the same style of `jq` invocation for both composer and osbuild commits (either both with `cat` or both directly on the file) and aligning the JSON paths if their structures are intended to be parallel.
## Individual Comments
### Comment 1
<location path="schutzbot/deploy.sh" line_range="192-193" />
<code_context>
-
-setup_repo osbuild-composer "${GIT_COMMIT}" 5
+# Get pinned commit for osbuild-composer to install RPMs
+COMPOSER_GIT_COMMIT=$(jq -r '.["'"${ID}-${VERSION_ID}"'"].dependencies.composer.commit' Schutzfile)
+setup_repo osbuild-composer "${COMPOSER_GIT_COMMIT}" 5
OSBUILD_GIT_COMMIT=$(cat Schutzfile | jq -r '.["'"${ID}-${VERSION_ID}"'"].dependencies.osbuild.commit')
</code_context>
<issue_to_address>
**issue:** Consider validating the composer commit from Schutzfile before passing it to setup_repo.
`get_last_passed_commit` used to guarantee a valid commit; now we trust the value from `Schutzfile` directly. If the key is missing or the value is `null`/empty, `setup_repo` will receive an invalid commit and fail later in a confusing way. Please add a check for `null`/empty here and fail fast with a clear error message mentioning the missing/invalid composer commit for `${ID}-${VERSION_ID}`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| COMPOSER_GIT_COMMIT=$(jq -r '.["'"${ID}-${VERSION_ID}"'"].dependencies.composer.commit' Schutzfile) | ||
| setup_repo osbuild-composer "${COMPOSER_GIT_COMMIT}" 5 |
There was a problem hiding this comment.
issue: Consider validating the composer commit from Schutzfile before passing it to setup_repo.
get_last_passed_commit used to guarantee a valid commit; now we trust the value from Schutzfile directly. If the key is missing or the value is null/empty, setup_repo will receive an invalid commit and fail later in a confusing way. Please add a check for null/empty here and fail fast with a clear error message mentioning the missing/invalid composer commit for ${ID}-${VERSION_ID}.
F-X64
left a comment
There was a problem hiding this comment.
I'm in favor of fixing the version.
I assume we will have an automatic PR as we do with the schutzfile?
Not yet, but I'll set it up. |
|
Actually, let me just add the scheduled job in this PR. No need to do it elsewhere. |
abb8d38 to
09a5bb2
Compare
|
Added script and action. |
thozza
left a comment
There was a problem hiding this comment.
In general, this looks good, but there are some copy&paste errors.
Pin the osbuild-composer commit to use so that it doesn't float and cause unexpected failures.
When deploying, read the commit ID from the Schutzfile to set up the osbuild-composer repository.
Adapted from the osbuild update script in osbuild/images [1]. Unlike the existing function in deploy.sh, it doesn't check for successful CI runs on the commit. It simply makes sure it doesn't use a post-release version bump commit (which don't run CI). Failed CI runs in osbuild-composer often happen after the RPM build stage and we only care about RPMs being available. Skipping a commit based on the overall GitLab CI run would skip over many valid commits. Cases where the PR is created pointing to a commit that doesn't have an RPM for a specific distro we need here can be dealt with manually (usually by retrying the CI job on the main branch of osbuild-composer). [1] https://github.com/osbuild/images/blob/8d0c776c4bf8687f413f3fcac2e28625bba2ce18/test/scripts/update-schutzfile-osbuild
Add a workflow that runs every Sunday to update the osbuild-composer commit ID in the Schutzfile.
09a5bb2 to
537d0aa
Compare
|
Thanks for catching those. Fixed and rebased. |
Pin the osbuild-composer commit to use so that it doesn't float and cause unexpected failures.
We will later add a cronjob to update the commit regularly, like we do with osbuild.