-
Notifications
You must be signed in to change notification settings - Fork 522
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
model: container-registry.mirrors
different settings representation
#1791
Conversation
sources/api/migration/migrations/v1.3.1/registry-mirror-representation/src/main.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this seems like a friendlier solution.
sources/api/migration/migrations/v1.3.1/registry-mirror-representation/src/main.rs
Outdated
Show resolved
Hide resolved
82c4bc1
to
cd7bee2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM assuming ECS testing is OK; couple small items remain above.
Changes the model for `container-registry.mirrors` from a `HashMap` to a vector of structs. Adds a custom deserializer to deserialize from either a TOML table or a TOML sequence.
Pushes above addresses @tjkirch 's comments. Adds more testing to the PR description. |
Push above removes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔩
Wow nice work!
sources/api/migration/migrations/v1.4.0/registry-mirror-representation/src/main.rs
Outdated
Show resolved
Hide resolved
faf9311
to
45b2cd2
Compare
Sorry about the repeat force pushes. Please take a look at the backward and forward migrations for the new changes. Pushes above addresses @bcressey 's comment. I also realized cases where the migration would fail if the registry contained Updated the PR description with updated migration testing info. |
(important change to code)
sources/api/migration/migrations/v1.4.0/registry-mirror-representation/src/main.rs
Show resolved
Hide resolved
sources/api/migration/migrations/v1.4.0/registry-mirror-representation/src/main.rs
Outdated
Show resolved
Hide resolved
sources/api/migration/migrations/v1.4.0/registry-mirror-representation/src/main.rs
Outdated
Show resolved
Hide resolved
push above addresses @tjkirch 's comments. I retested the migrations and they worked as expected.
On next boot:
|
Push above removes an outdated comment and a redundant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧦
Issue number:
Fixes #1780
Description of changes:
Testing done:
Unit test passes.
On a live host, I can specify registry mirrors in both ways. Either as a map of registry to endpoints or an array of tables with
registry
andendpoints
as object fields.With the following userdata:
On K8s variants, the containerd config registry mirrors configuration gets rendered correctly:
On ECS variants, the docker daemon json file generated correctly for
docker.io
registry mirrors and docker runs fine:userdata:
docker
daemon.json
:host-containers services registry mirrors config also gets generated correctly:
Test migration upgrades and downgrades and the
container-registry.mirrors
setting gets migrated successfully:With the following userdata:
On current version Bottlerocket:
Checking after upgrade:
Checking containerd config:
After downgrading back:
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.