Skip to content

Conversation

@joelcapitao
Copy link
Member

@joelcapitao joelcapitao commented Dec 18, 2025

We want to use the lockfiles generated by the cmd-import command into our bump-lockfile Jenkins job.
If the job is successful then we commit those files into the config repo.
So we need them to be properly formatted/indented.

We want to use the lockfiles generated by the cmd-import command
into our bump-lockfile Jenkins job. If the job is successful then
we commit those files into the config repo. So we need them to
be properly formatted/indented.
Copy link
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 correctly adds indentation to the lockfile generated by src/cmd-import, which improves readability and consistency for downstream tooling. The change is simple and effective. I've added one suggestion to refactor the json.dump call slightly to improve consistency with other calls in the file, which will enhance maintainability.

For future improvements, you might also consider adding indentation to other json.dump calls in the codebase to ensure consistency. I've noticed a few places that could benefit from this in a follow-up PR:

  • src/cmdlib.sh, line 471 (for local-overrides.json)
  • src/cmdlib.sh, line 661 (in strip_out_lockfile_arches)
  • src/cosalib/cmdlib.py, line 537 (for image.json)
  • src/cosalib/builds.py, line 158 (for meta.json)

Comment on lines +155 to 158
json.dump(fp=f, indent=2, obj={
'packages': rpmdb,
'metadata': {'generated': rfc3339_time(set_midnight=True)}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

While this change is functionally correct, the way json.dump is called is inconsistent with other calls in this file (e.g., on line 152 and 232), which use positional arguments for obj and fp. For better readability and long-term maintainability, it's best to stick to a consistent style. I'd recommend using positional arguments here as well.

Suggested change
json.dump(fp=f, indent=2, obj={
'packages': rpmdb,
'metadata': {'generated': rfc3339_time(set_midnight=True)}
})
obj = {
'packages': rpmdb,
'metadata': {'generated': rfc3339_time(set_midnight=True)}
}
json.dump(obj, f, indent=2)

Copy link
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 dustymabe enabled auto-merge (rebase) December 18, 2025 14:34
@dustymabe dustymabe disabled auto-merge December 18, 2025 18:52
@dustymabe dustymabe merged commit 1420036 into coreos:main Dec 18, 2025
5 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.

2 participants