Skip to content
New issue

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

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

Already on GitHub? # to your account

Update the way we're getting a container's hostname #352

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

JacobCallahan
Copy link
Member

Previously, if a user set a hostname on a container, Broker wouldn't recognize that. This is because we just looked up the container's id. When a hostname isn't set at runtime, this is perfectly fine. However, when a user does set a hostname, this breaks what would be expected.

With this change, we have a more reliable way to solve both cases.

Previously, if a user set a hostname on a container, Broker wouldn't
recognize that. This is because we just looked up the container's id.
When a hostname isn't set at runtime, this is perfectly fine.
However, when a user does set a hostname, this breaks what would be
expected.

With this change, we have a more reliable way to solve both cases.
@JacobCallahan JacobCallahan added the enhancement New feature or request label Feb 19, 2025
@JacobCallahan JacobCallahan self-assigned this Feb 19, 2025

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

broker/providers/container.py:25

  • Ensure that the new method of fetching the hostname from container attributes is covered by tests.
"hostname": container_inst.attrs["Config"].get("Hostname"),

broker/providers/container.py:211

  • Ensure that the new method of fetching the hostname from container attributes is covered by tests.
hostname = cont_inst.attrs["Config"].get("Hostname")
@aadhikar
Copy link

LGTM!

@JacobCallahan
Copy link
Member Author

Tests currently failing due to: containers/podman-py#514, which @jyejare has already resolved. Hopefully we will get a new build of podman-py soon. However, I won't block this PR until then.

Manual testing of the changes show it working both for @aadhikar and myself.

❯ broker checkout --container-host localhost/ubi9:latest --hostname second.test.hostname
[INFO 250219 09:52:39] Using provider Container to checkout
[INFO 250219 09:52:39] Host: second.test.hostname

and in the inventory

4:
  name: jake_812af936
  _broker_provider_instance: podman_ipv4
  type: host
  hostname: second.test.hostname
  _broker_provider: Container
  _broker_args:
    container_host: localhost/ubi9:latest
    hostname: second.test.hostname
  exposed_ports: {

and now on the container itself

[root@infra-podman-ipv4 ~]# podman exec -it jake_812af936 /bin/bash
[root@second ~]# hostname
second.test.hostname

@jyejare
Copy link
Member

jyejare commented Feb 20, 2025

@JacobCallahan @aadhikar The podman-py new version is released with fix. I have rerun all the checks for this PR.

@jyejare jyejare merged commit 426c234 into SatelliteQE:master Feb 20, 2025
5 checks passed
@JacobCallahan JacobCallahan deleted the container_hostname branch February 20, 2025 14:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants