Skip to content
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.

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

fix containerd config generation #550

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

vflaux
Copy link

@vflaux vflaux commented Jul 29, 2024

This is proposal to fix #549.
This is a bit complicated in my opinion but I couldn't figure out a better way.
Except by writing a toml file without a toml lib which should be simpler and maybe preferable?

Map are output as toml table sorted by the key, which is not what we expect.
This patch dynamically build a struct with a field for each host (and a toml tag).
This allow us to control the order of hosts in the toml file as go-toml output struct as toml tables while preserving the order of the fields in the struct.

@vflaux vflaux marked this pull request as ready for review July 29, 2024 17:03
@vflaux vflaux force-pushed the fix-549 branch 5 times, most recently from 08f87c1 to 1c5d335 Compare July 30, 2024 13:57
@phillebaba
Copy link
Member

Thank you for reporting the problem and proposing a solution. I dont really know why I never thought about this being a problem. It is slightly weird that the order of maps actually matter. There is something about your solution that I feel makes things more complicated than it has to be. I will dig a bit and see if there is a easier solution, if not we will merge yours for the time being.

The good thing is that we have tests that validate this for future changes.

Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
pkg/oci/containerd.go 63.80% <100.00%> (+4.21%) ⬆️

@vflaux
Copy link
Author

vflaux commented Aug 6, 2024

I opened a new PR with an alternative implementation using go template : #557.
It's less complicated than this one.

It is slightly weird that the order of maps actually matter.

That's quite unexpected and a bad choice IMO, they should have used an array instead. Containerd also need to parse the file a second time to get the order : https://github.com/containerd/containerd/blob/7f707b5e7970105723257d483394454049eabe47/core/remotes/docker/config/hosts.go#L519.

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.

Containerd hosts config are sorted and not in the specified order
2 participants