10000 fix: remove nested structs from configuration by shanduur · Pull Request #4523 · distribution/distribution · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: remove nested structs from configuration #4523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 9, 2025
Merged

fix: remove nested structs from configuration #4523

merged 1 commit into from
Mar 9, 2025

Conversation

shanduur
Copy link
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
Member

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
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
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
Member

This needs a rebase now @shanduur

milosgajdos
milosgajdos reviewed Dec 18, 2024
View reviewed changes
10000
Copy link
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
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
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
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
Member

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
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
0