Skip to content

Conversation

@jlebon
Copy link
Member

@jlebon jlebon commented Aug 25, 2025

This is older than coreos-assembler.container-config-git and is used in various places in the pipeline/CI.

This is older than coreos-assembler.container-config-git and is used in
various places in the pipeline/CI.
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 the coreos-assembler.config-gitrev metadata field during OCI image import, ensuring compatibility with older pipeline components. The change is straightforward and well-implemented. I have one suggestion to improve maintainability by replacing magic strings for dictionary keys with constants.

Comment on lines 149 to +156
source = metadata.get('Labels', {}).get('org.opencontainers.image.source')
commit = metadata.get('Labels', {}).get('org.opencontainers.image.revision')
if source and commit:
meta['coreos-assembler.container-config-git'] = {
'origin': source,
'commit': commit,
}
meta['coreos-assembler.config-gitrev'] = commit
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve maintainability and avoid potential typos from magic strings, consider defining the dictionary keys used here as constants. This is particularly useful for keys that are part of a specification (like OCI labels) or are shared across different parts of the codebase.

You could define them at the top of the file, for example:

# OCI image spec labels
OCI_LABEL_SOURCE = 'org.opencontainers.image.source'
OCI_LABEL_REVISION = 'org.opencontainers.image.revision'

# COSA metadata keys
COSA_META_CONTAINER_CONFIG_GIT = 'coreos-assembler.container-config-git'
COSA_META_CONFIG_GITREV = 'coreos-assembler.config-gitrev'

And then use these constants in this block:

    source = metadata.get('Labels', {}).get(OCI_LABEL_SOURCE)
    commit = metadata.get('Labels', {}).get(OCI_LABEL_REVISION)
    if source and commit:
        meta[COSA_META_CONTAINER_CONFIG_GIT] = {
            'origin': source,
            'commit': commit,
        }
        meta[COSA_META_CONFIG_GITREV] = commit

@dustymabe
Copy link
Member

This is older than coreos-assembler.container-config-git and is used in various places in the pipeline/CI.

Does this advocate for us finding and fixing those few places rather than adding 2nd order backwards compat?

dustymabe added a commit to dustymabe/coreos-ci that referenced this pull request Aug 25, 2025
The .config-gitrev is legacy. Let's update to use the newer API.

xref: coreos/coreos-assembler#4282
dustymabe added a commit to dustymabe/fedora-coreos-pipeline that referenced this pull request Aug 25, 2025
The .config-gitrev is legacy. Let's update to use the newer API.

xref: coreos/coreos-assembler#4282
@dustymabe
Copy link
Member

This is older than coreos-assembler.container-config-git and is used in various places in the pipeline/CI.

Does this advocate for us finding and fixing those few places rather than adding 2nd order backwards compat?

At least here is a start:

dustymabe added a commit to dustymabe/fedora-coreos-browser that referenced this pull request Aug 25, 2025
The .config-gitrev is legacy. Let's update to use the newer API.

xref: coreos/coreos-assembler#4282
@dustymabe
Copy link
Member

@jlebon
Copy link
Member Author

jlebon commented Aug 25, 2025

Yup, sure. Trying to fix clients SGTM too.

@jlebon jlebon closed this Aug 25, 2025
jlebon pushed a commit to coreos/fedora-coreos-pipeline that referenced this pull request Aug 25, 2025
The .config-gitrev is legacy. Let's update to use the newer API.

xref: coreos/coreos-assembler#4282
jlebon pushed a commit to coreos/coreos-ci that referenced this pull request Aug 25, 2025
The .config-gitrev is legacy. Let's update to use the newer API.

xref: coreos/coreos-assembler#4282
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