Skip to content

fix: nil dereference in archive priority processing - #269

Merged
niemeyer merged 10 commits into
canonical:mainfrom
lczyk:panic-for-invalid-archives
Mar 16, 2026
Merged

fix: nil dereference in archive priority processing#269
niemeyer merged 10 commits into
canonical:mainfrom
lczyk:panic-for-invalid-archives

Conversation

@lczyk

@lczyk lczyk commented Mar 3, 2026

Copy link
Copy Markdown
Contributor
  • Have you signed the CLA?

very niche, but the test by itself leads to a panic since we collect keys from yamlArchives but then index release.Archives with them

@lczyk

lczyk commented Mar 3, 2026

Copy link
Copy Markdown
Contributor Author

sorry for the empty commit noise. the github workers were loosing their minds and i needed to retrigger a couple of times

@lczyk
lczyk requested a review from upils March 3, 2026 22:42
@lczyk lczyk added Simple Nice for a quick look on a minute or two Bug An undesired feature ;-) labels Mar 3, 2026
@github-actions

github-actions Bot commented Mar 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Command Mean [s] Min [s] Max [s] Relative
BASE 13.657 ± 0.084 13.553 13.783 1.00 ± 0.01
HEAD 13.624 ± 0.061 13.523 13.700 1.00

upils
upils suggested changes Mar 4, 2026
View reviewed changes

@upils upils left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @lczyk, good catch!

The test just need a bit of tuning and then it shall be good to move to core review.

Comment thread internal/setup/setup_test.go Resolved
Comment thread internal/setup/setup_test.go Resolved
@lczyk

lczyk commented Mar 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@upils both comments addressed. i wasn't 100% sure where to put the test, but i think i found a decent place.

also, i realized there is no equivalent test for v2/v3 so i've added that too (it's passing without any changes. the bug addressed here is with v1): df1581d

@lczyk
lczyk requested a review from upils March 4, 2026 12:43
upils
upils suggested changes Mar 6, 2026
View reviewed changes

@upils upils left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for rework @lczyk. See my other comment regarding the tests.

Comment thread internal/setup/setup_test.go Outdated
},
relerror: `chisel.yaml: cannot parse maintenance: expected format for "standard" is YYYY-MM-DD`,
}, {
summary: "Maintenance: invalid pro in v2-archives ignored",

@upils upils Mar 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe this test is not tied to the "maintenance" feature. Unless I am missing something, it looks like the name (and the content of the chisel.yaml are making it difficult to actually know what is tested.

Also, to be inline with the last thing discussed in #267, I suppose a single test for V3 would be enough since the tested behavior is common for all formats.

@lczyk lczyk Mar 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe this test is not tied to the "maintenance" feature.

ah, sorry, i thought Maintenance: ... meant tests of v1. changed in 9bbad89

it looks like the name (and the content of the chisel.yaml are making it difficult to actually know what is tested.

that's the smallest yaml which was causing the bug... 🤷‍♀️

i've added a comment to point the reader at the point of the test 768d763

I suppose a single test for V3 would be enough since the tested behavior is common for all formats.

the tests test different things. might be clearer now that the tests are next to one another + the name is reworded. also, importantly, without

- for archiveName := range yamlArchives {
+ for archiveName := range release.Archives {

only one of the tests fails ( only the v2-archive one ). hence, i believe there is a strong reason to keep both.

@upils upils Mar 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@lczyk I think you missed this comment. If not, would you mind exposing why you think the second test must be kept?

@lczyk lczyk Mar 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

see above. sorry, stupid github decided to mark the comments as pending from some reason and never actually make them... 😠

@lczyk
lczyk requested a review from upils March 6, 2026 12:40
upils
upils suggested changes Mar 10, 2026
View reviewed changes

@upils upils left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks but see the previous comment I pinged you on (#269 (comment))

upils
upils suggested changes Mar 12, 2026
View reviewed changes

@upils upils left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the improved test. I see now why 2 tests make sense.

See my other comment about tests location.

Comment thread internal/setup/setup_test.go Resolved
@niemeyer
niemeyer merged commit dc93148 into canonical:main Mar 16, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@upils upils upils requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug An undesired feature ;-) Simple Nice for a quick look on a minute or two

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lczyk @upils @niemeyer