feat(config): Add fingerprint ignore tags to some config fields#46
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a fingerprint:"-" struct tag on selected internal/projectconfig fields to mark them as excluded from “component fingerprinting”, and adds a test to ensure the exclusion list stays consistent over time.
Changes:
- Add
fingerprint:"-"tags to multiple config fields that are intended to be treated as non-build-input metadata. - Add a reflection-based unit test to detect accidental additions/removals of
fingerprint:"-"exclusions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/projectconfig/overlay.go | Excludes ComponentOverlay.Description from the fingerprint via tag. |
| internal/projectconfig/build.go | Excludes check/failure/hints “reason/policy” fields from the fingerprint via tag. |
| internal/projectconfig/component.go | Excludes bookkeeping/back-reference fields and SourceFileReference.Origin from the fingerprint via tag. |
| internal/projectconfig/distro.go | Excludes DistroReference.Snapshot from the fingerprint via tag. |
| internal/projectconfig/fingerprint_test.go | Adds a test that asserts the expected set of fingerprint exclusions. |
|
|
||
| // Origin for this source file. When omitted, the file is resolved via the lookaside cache. | ||
| Origin Origin `toml:"origin,omitempty" json:"origin,omitempty"` | ||
| Origin Origin `toml:"origin,omitempty" json:"origin,omitempty" fingerprint:"-"` |
There was a problem hiding this comment.
Excluding SourceFileReference.Origin from the fingerprint assumes the source file’s content is fully captured by Filename + Hash/HashType, but hash/hash-type are optional and downloads proceed without validation when they’re empty (see downloadAndValidate in internal/providers/sourceproviders/sourcemanager.go). If the fingerprint is used to decide whether sources/build outputs are up-to-date, consider including Origin when hash info is missing, or enforce that hash + hash-type are required for source-files entries so changes in origin can’t silently change inputs without changing the fingerprint.
| Origin Origin `toml:"origin,omitempty" json:"origin,omitempty" fingerprint:"-"` | |
| Origin Origin `toml:"origin,omitempty" json:"origin,omitempty"` |
There was a problem hiding this comment.
Will add a check to the calculation step that errors on this case
There was a problem hiding this comment.
| Version string `toml:"version,omitempty" json:"version,omitempty" jsonschema:"title=Version,description=Version of the referenced distro"` | ||
| // Snapshot date/time for source code if specified components will use source as it existed at this time. | ||
| Snapshot string `toml:"snapshot,omitempty" json:"snapshot,omitempty" jsonschema:"format=date-time,title=Snapshot,description=If specified use source code as it existed at this date/time"` | ||
| Snapshot string `toml:"snapshot,omitempty" json:"snapshot,omitempty" jsonschema:"format=date-time,title=Snapshot,description=If specified use source code as it existed at this date/time" fingerprint:"-"` |
There was a problem hiding this comment.
DistroReference.Snapshot directly affects which upstream commit is checked out when upstream-commit isn’t set (see checkoutTargetCommit in internal/providers/sourceproviders/fedorasourceprovider.go). Ignoring snapshot in the fingerprint can therefore miss real input changes when the snapshot time changes. If you intend to keep snapshot out of the fingerprint, the resolved commit hash used for the checkout should be captured elsewhere and included in the fingerprint/state instead.
| Snapshot string `toml:"snapshot,omitempty" json:"snapshot,omitempty" jsonschema:"format=date-time,title=Snapshot,description=If specified use source code as it existed at this date/time" fingerprint:"-"` | |
| Snapshot string `toml:"snapshot,omitempty" json:"snapshot,omitempty" jsonschema:"format=date-time,title=Snapshot,description=If specified use source code as it existed at this date/time"` |
There was a problem hiding this comment.
The fully resolved commit for each component is part of the fingerprint. That is more reliable than the snapshot date (also, updating the snapshot date would cause a rebuild of every single package, even if many of them didn't change).
There was a problem hiding this comment.
dmcilvaney
force-pushed
the
damcilva/component_change_detection_parts/4
branch
from
daeff90 to
33ef430
Compare
|
Should add a blurb in copilot instructions about the fingerprint tag. |
|
Test failure looks transient and unrelated to changes |
dmcilvaney
force-pushed
the
damcilva/component_change_detection_parts/4
branch
from
d298954 to
44142f2
Compare
| } | ||
| ``` | ||
|
|
||
| ## Component Fingerprinting — `fingerprint:"-"` Tags |
There was a problem hiding this comment.
Can we get a general overview of this new tag added to some of the human-targeted docs too?
There was a problem hiding this comment.
Final PR has a section on it, figured I'd put the whole feature set in a doc.
There was a problem hiding this comment.
We should document as we go. It's clear directions change quickly here.
There was a problem hiding this comment.
moved docs up to this PR
In preparation for the upcoming component change detection feature, this commit selects which config fields should be ignored when computing the identity of a component using a fingerprinting mechanism.
dmcilvaney
force-pushed
the
damcilva/component_change_detection_parts/4
branch
from
6860659 to
7c7a218
Compare
Prepare for fingerprint calculations in #47