Skip to content

cmdlib.py: minor tweaks related to OCI imports#4227

Merged
dustymabe merged 2 commits into
coreos:mainfrom
jlebon:pr/import-followups
Jul 18, 2025
Merged

cmdlib.py: minor tweaks related to OCI imports#4227
dustymabe merged 2 commits into
coreos:mainfrom
jlebon:pr/import-followups

Conversation

@jlebon
Copy link
Copy Markdown
Member

@jlebon jlebon commented Jul 18, 2025

Address some comments from #4164.

@jlebon jlebon mentioned this pull request Jul 18, 2025
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request renames a variable for clarity and adds comments to improve code understanding. A suggestion is made to convert a comment block into a docstring for better documentation.

Comment thread src/cosalib/cmdlib.py Outdated
@jlebon jlebon force-pushed the pr/import-followups branch from f5d28ce to 4563969 Compare July 18, 2025 15:48
dustymabe
dustymabe previously approved these changes Jul 18, 2025
Copy link
Copy Markdown
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks!

jbtrystram
jbtrystram previously approved these changes Jul 18, 2025
We had this really subtle interaction going on between cosa build and
prune, where we were actually relying on the prune code to regenerate
`builds.json` to add the new build we just did. There's history there
on how we got here, but at this point it just feels weird. It's cleaner
to always insert the new build ourselves and then separately call
`cosa prune` so let's do that.
@jlebon jlebon dismissed stale reviews from jbtrystram and dustymabe via d1f2622 July 18, 2025 18:08
@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Jul 18, 2025

OK wow, the new code in #4164 unearthed funky semantics in cmd-build. Added another commit to fix that which should fix CI.

Copy link
Copy Markdown
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dustymabe
Copy link
Copy Markdown
Member

I'm not even able to get to prow so I'm skipping CI on ci/prow/rhcos to get this fix in.

See coreos/fedora-coreos-config#3605 (comment)

@dustymabe dustymabe enabled auto-merge (rebase) July 18, 2025 20:27
@dustymabe dustymabe disabled auto-merge July 18, 2025 20:27
@dustymabe dustymabe merged commit b9f7e78 into coreos:main Jul 18, 2025
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants