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

[release/1.7] Fix issue with empty host tree in hosts.toml #10028

Open
wants to merge 2 commits into
base: release/1.7
Choose a base branch
from

Conversation

brandond
Copy link
Contributor

@brandond brandond commented Apr 1, 2024

Fixes: #10027

@k8s-ci-robot
Copy link

Hi @brandond. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
@dmcgowan
Copy link
Member

dmcgowan commented Apr 2, 2024

Seems the issue here would be better fixed by just changing getSortedhosts to return an empty list in that case. What is root.Get("host") returning when you provide it the [host] config? It seems that code should handle root.Get("host") returning nil, but is that what it sees.

@brandond
Copy link
Contributor Author

brandond commented Apr 2, 2024

Seems the issue here would be better fixed by just changing getSortedhosts to return an empty list in that case.

Sure, that'd work too. It didn't seem like there was much point in calling getSortedHosts at all if there are no HostConfigs in the first place, but what you're suggesting is probably a slightly less involved change.

What is root.Get("host") returning when you provide it the [host] config?

I don't know, I didn't poke at the toml parser return; I suspect it just returns an empty tree so that list := append([]string{}, iter.Keys()...) just ends up creating an empty list, which is what gets returned.

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
@dmcgowan dmcgowan changed the title Fix issue with empty host tree in hosts.toml [release/1.7] Fix issue with empty host tree in hosts.toml Apr 5, 2024
@dmcgowan
Copy link
Member

Note this fix is not needed in main as we have bumped the toml library major version and the new implementation does not have this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants