Skip to content

Fixes and improvements#2947

Merged
milas merged 7 commits into
docker:masterfrom
kinday:various-fixes
Jul 26, 2022
Merged

Fixes and improvements#2947
milas merged 7 commits into
docker:masterfrom
kinday:various-fixes

Conversation

@kinday
Copy link
Copy Markdown
Contributor

@kinday kinday commented Feb 2, 2022

This PR fixes various issues and introduces CI improvements. There are no functional changes, only stylistic and/or infrastructure fixes. Issues were encountered during work on other contribution when I attempted to run make test.

Fix flake8 errors (e1a203f)

make test was failing due to several formatting issues, mostly line width. I have reformatted code in multiple places to make flake8 happy1.

Fix integration tests race condition (89bce10)

Integration tests were crashing as they were starting before Docker-in-Docker container was ready to accept connections. This was reported in #996 and sleep is not a viable fix as DinD startup time varies.

I have updated Makefile2 to run netcat in busybox image. This solution is used instead of running netcat on host system for two reasons: the binary might not be present and container could be unreachable from host network3

Run integration tests on CI (d28f522)

Integration test suite does not fully pass on macOS thus I have updated workflow to run them on CI. This could help people with compatibility issues and ensure that proposed changes are not breaking anything.

Update: I’ve noticed integration tests are being run on Jenkins. Maybe this one could be removed, but running them via GHA allows to get them executed before PR was created, i.e. in contributor’s repository and give early feedback.

Use existing DIND version (924e09a)

The 20.10.05 tag is no longer available on Docker Hub. I have replaced it with 20.10 as this seems to be more resilient than specifying full tag.

Footnotes

  1. I am not sure if I formatted the code according to Python code style; please tell if it should it be indented/splitted differently.

  2. I took liberty to partially reformat Makefile and replace shorthand flags with longhand ones for readability. I can either revert the changes, or apply the same style to whole file or keep things as is. Your call.

  3. Could be observed on macOS as Docker is being run in virtual machine and the only way to access container is to bind ports to host network.

Signed-off-by: Leonard Kinday <leonard@kinday.ru>
Signed-off-by: Leonard Kinday <leonard@kinday.ru>
Signed-off-by: Leonard Kinday <leonard@kinday.ru>
Signed-off-by: Leonard Kinday <leonard@kinday.ru>
Comment thread docker/api/image.py Outdated
Comment on lines +383 to +384
decode=True
):
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should probably look like this according to other places:

Suggested change
decode=True
):
decode=True):

Comment thread docker/api/container.py Outdated
Comment on lines +259 to +262
client.api.create_host_config(port_bindings={1111: (
'127.0.0.1',
4567
)})
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should probably look like this according to other places:

Suggested change
client.api.create_host_config(port_bindings={1111: (
'127.0.0.1',
4567
)})
client.api.create_host_config(
port_bindings={1111: ('127.0.0.1', 4567)})

milas added 3 commits July 26, 2022 15:55
@milas
Copy link
Copy Markdown
Contributor

milas commented Jul 26, 2022

Thank you for the fixes and improvements! I just brought this branch up to date. The integration test wait fix is much appreciated ✌️

@milas milas merged commit 0ee9f26 into docker:master Jul 26, 2022
@milas milas self-assigned this Jul 26, 2022
@milas milas added this to the 6.0.0 milestone Jul 30, 2022
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.

2 participants