Skip to content

fix: remove nested structs from configuration#4523

Merged
milosgajdos merged 1 commit intodistribution:mainfrom
shanduur:fix/define-types
Mar 9, 2025
Merged

fix: remove nested structs from configuration#4523
milosgajdos merged 1 commit intodistribution:mainfrom
shanduur:fix/define-types

Conversation

@shanduur
Copy link
Copy Markdown
Contributor

@shanduur shanduur commented Dec 5, 2024 •
edited
Loading

Hi! I've started designing Kubernetes Operator for Distribution (see https://registry-operator.dev/latest/), and I noticed that handling the configuration structure from code is extremely cumbersome due to the nested structs. Simplest fix is to define few new structures, and unflatten the configuration.

Progress:

  • unflatten the code
  • godoc
  • tests

@github-actions github-actions bot added area/config Related to registry config dependencies Pull requests that update a dependency file labels Dec 5, 2024
@milosgajdos
Copy link
Copy Markdown
Member

milosgajdos commented Dec 5, 2024

I noticed you're attempting to bump the yaml module to v3. It's not as straightforward as it might seem. There is an existing PR #4178 which is marked as a draft due to its scope outgrowing the original estimate; besides, if I remember correctly, in order to bump to v3 there would have to be further changes made to the core of the configuration package which would grow the scope of the change further still.

@shanduur
Copy link
Copy Markdown
Contributor Author

shanduur commented Dec 5, 2024

Yup, gonna drop this for now.

@shanduur shanduur force-pushed the fix/define-types branch 2 times, most recently from ed9a7cd to e8ef767 Compare December 5, 2024 16:00
@shanduur shanduur marked this pull request as ready for review December 5, 2024 18:30
milosgajdos
milosgajdos previously approved these changes Dec 6, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@milosgajdos milosgajdos left a comment

Choose a reason for hiding this comment

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

Thanks! PTAL @thaJeztah

@milosgajdos
Copy link
Copy Markdown
Member

milosgajdos commented Dec 17, 2024

This needs a rebase now @shanduur

milosgajdos
milosgajdos reviewed Dec 18, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@milosgajdos milosgajdos left a comment

Choose a reason for hiding this comment

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

This broke the build now

@shanduur
Copy link
Copy Markdown
Contributor Author

shanduur commented Mar 6, 2025

@milosgajdos sorry that it took so long, it should be fixed now. I've rebased the PR, PTAL. If any fixes are needed, I'd be more than happy to do it!

@shanduur shanduur requested a review from milosgajdos March 6, 2025 19:28
milosgajdos
milosgajdos approved these changes Mar 7, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@milosgajdos milosgajdos left a comment

Choose a reason for hiding this comment

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

LGTM. PTAL @thaJeztah @Jamstah @wy65701436

davidspek
davidspek approved these changes Mar 9, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@davidspek davidspek left a comment

Choose a reason for hiding this comment

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

Better structure is always good!

@milosgajdos
Copy link
Copy Markdown
Member

milosgajdos commented Mar 9, 2025

Can you rebase (squash commits) @shanduur; otherwise we're good to merge!

@shanduur
fix: define types for inline structs
b2cc4fe 
Signed-off-by: Mateusz Urbanek <mateusz.urbanek.98@gmail.com>
@shanduur
Copy link
Copy Markdown
Contributor Author

shanduur commented Mar 9, 2025

@milosgajdos squashed.

@milosgajdos milosgajdos merged commit 4974b85 into distribution:main Mar 9, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@milosgajdos milosgajdos milosgajdos approved these changes

@davidspek davidspek davidspek approved these changes

@Jamstah Jamstah Awaiting requested review from Jamstah

@thaJeztah thaJeztah Awaiting requested review from thaJeztah

@squizzi squizzi Awaiting requested review from squizzi

@joaodrp joaodrp Awaiting requested review from joaodrp

@wy65701436 wy65701436 Awaiting requested review from wy65701436

Assignees

No one assigned

Labels

area/config Related to registry config dependencies Pull requests that update a dependency file refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shanduur @milosgajdos @davidspek