[mitar]

When the user does not explicitly set a hostname, Docker auto-assigns one based on the container's ID, so the existing and temporary containers always disagree on Hostname even when nothing has actually changed. In that case we must ignore Hostname to avoid spurious recreations. When the user has set a hostname, we need to compare it so that changing the "hostname" argument triggers a container replacement.

Merge requirements satisfied?

[NOTICE] Bug fixes or new features require tests.

• Docs
• Changelog - https://salt-extensions.github.io/salt-extension-copier/topics/documenting/changelog.html#procedure
• Tests written/updated

Commits signed with GPG?

No

[codecov]

Codecov Report

❌ Patch coverage is 64.42308% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.22%. Comparing base (3bc2a01) to head (6c19488).

Files with missing lines	Patch %	Lines
tests/unit/states/test_docker_container.py	45.45%	36 Missing ⚠️
src/saltext/dockermod/states/docker_container.py	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
+ Coverage   42.88%   43.22%   +0.34%     
==========================================
  Files          26       27       +1     
  Lines        6564     6665     +101     
  Branches     1122     1125       +3     
==========================================
+ Hits         2815     2881      +66     
- Misses       3494     3529      +35     
  Partials      255      255              

Flag	Coverage Δ
Linux	43.21% <64.42%> (+0.34%)	⬆️
project	38.53% <88.88%> (+0.10%)	⬆️
py310	43.16% <64.42%> (+0.34%)	⬆️
py311	43.16% <64.42%> (+0.34%)	⬆️
py314	43.16% <64.42%> (+0.34%)	⬆️
salt_3006_27	43.16% <64.42%> (+0.34%)	⬆️
salt_3008_2	43.16% <64.42%> (+0.34%)	⬆️
tests	51.78% <62.10%> (+0.43%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[m-czernek]

Is this a cherry-pick from Salt, or new commits? I saw you mentioned missing commits in one of the issues, but neither 3006.x [0] nor 3007.x has these changes.

[0] https://github.com/saltstack/salt/blob/3006.x/salt/states/docker_container.py#L1859
[1] https://github.com/saltstack/salt/blob/3007.x/salt/states/docker_container.py#L1852

[mitar]

These are new commits. They are from my fork where I was collecting fixes I made while this repo was not populated with docker code. Now that it is, I decided to propose those fixes upstream (i.e., here).

The ones I listed as missing commits I didn't make PRs for. I can, but I thought that it is probably easier for you to just do those.

[ycedres]

Than you for the PR. I was thinking about this use case

node_step1_clean:
  docker_container.absent:
    - name: node-01
    - force: True

node_step1_prod:
  docker_container.running:
    - name: node-01
    - image: alpine:latest
    - command: sleep 30
    - require:
      - docker_container: node_step1_clean

node_step2_debug:
  docker_container.running:
    - name: node-01
    - image: alpine:latest
    - command: sleep 30
    - hostname: debug-node-01
    - require:
      - docker_container: node_step1_prod

node_step3_restore:
  docker_container.running:
    - name: node-01
    - image: alpine:latest
    - command: sleep 30
    - require:
      - docker_container: node_step2_debug

So you first let dockermod to set a hostname based on the container ID, later you actively rename it for some task and then you want to have back the default behaviour. I think in node_step3_restore the container keeps the hostname debug-node-01 and this is not intended. Does this make sense to you?

[mitar]

You are right. My fix fixed on part, but then opened another issue. I think it is all now addressed.