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.

Sign up for GitHub

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

Jump to bottom

feature(Docker): Add support for platform parameter #95

Merged
merged 11 commits into from
Apr 5, 2023
Merged

feature(Docker): Add support for platform parameter #95

merged 11 commits into from
Apr 5, 2023

Conversation

dankow
Copy link
Contributor

@dankow dankow commented Jan 26, 2023

Pass platform value from platforms.item to docker_container. Example usage:

driver:
  name: docker
platforms:
  - name: test-01
    image: ...
    pre_build_image: true
    platform: linux/amd64

As suggested in the docker_container documentation, ignore the platform parameter when comparing state.

Addresses issue #94.

@dankow
Docker: Add support for platform parameter
d4945e8 
Pass `platform` value from `platforms.item` to `docker_container`. Example usage:
```
driver:
  name: docker
platforms:
  - name: test-01
    image: ...
    pre_build_image: true
    platform: linux/amd64
```

As suggested in the [docker_container documentation](https://docs.ansible.com/ansible/latest/collections/community/docker/docker_container_module.html#ansible-collections-community-docker-docker-container-module), ignore the `platform` parameter when comparing state.

Addresses issue ansible-community#94.
@Maddog2050 Maddog2050 mentioned this pull request Feb 16, 2023
Closed
@ThomasSteinbach
Copy link

@dankow I am no reviewer but I would like to see your feature merged. I recognized, that some checks failed. Please fix following things, to hopefully get your PR merged:

@dankow dankow changed the title Docker: Add support for platform parameter feature(Docker): Add support for platform parameter Mar 1, 2023
@dankow
Copy link
Contributor Author

dankow commented Mar 1, 2023

@ThomasSteinbach Thanks for the hint, I've changed the PR title

@ThomasSteinbach
Copy link

@ssbarnea would it be possible to review/merge that PR.
Without that change Molecule actually didn't work on M1 Macs.
Thanks in advance.

@ssbarnea
Copy link
Member

ssbarnea commented Mar 4, 2023

This needs fixing, failed even linting.

@dankow
Copy link
Contributor Author

dankow commented Mar 4, 2023

@ssbarnea - Correct me if I'm wrong, but aren't the linting failures and other failures in completely unrelated parts of the code?

@ThomasSteinbach
Copy link

@ssbarnea I have to agree to @dankow , none of the current pull requests passes the checks.
The output of those checks seems to me quite unreleated to the changes in this PR, e.g. linting:

yaml[indentation]: Wrong indentation: expected at least 3
test/gce/scenarios/windows/verify.yml:8

... or in pytests ...

STDERR: ERROR! couldn't resolve module/action 'containers.podman.podman_image'

I don't know if rebasing to main would help. Does it @ssbarnea ?

@apatard
Copy link
Member

apatard commented Mar 7, 2023

@ssbarnea I have to agree to @dankow , none of the current pull requests passes the checks. The output of those checks seems to me quite unreleated to the changes in this PR, e.g. linting:

yaml[indentation]: Wrong indentation: expected at least 3
test/gce/scenarios/windows/verify.yml:8

... or in pytests ...

STDERR: ERROR! couldn't resolve module/action 'containers.podman.podman_image'

I don't know if rebasing to main would help. Does it @ssbarnea ?

I can't talk for @ssbarnea who is the maintainer of this project but what I can say:

So, imho, the timeouts has to be fixed first, then PR101 tests can be restarted and hopefully this PR merged. Once done, main branch should have CI in green. Then, pending pull requests will have to be rebased and hopefully may be merged if review is fine. Blindly merging PR without CI "because CI is broken" seems really bad practice to me...

@apatard
Copy link
Member

apatard commented Mar 23, 2023

@dankow Hi. thanks to some other devs, the CI seems to be on happy side these days. Can you please rebase and check new CI status ?

zhan9san and others added 9 commits March 31, 2023 11:28
@zhan9san @dankow
Fix ci issue (ansible-community#111)
c92caf0 
* Fix ci issue

* fix ci
@zhan9san @dankow
Optimize CI (ansible-community#112)
caeb2be 
Install collection from galaxy, fix typo, do not install ansible via apt-get
@apatard @dankow
README.md: Update plugin list (ansible-community#91)
78828cb 
With newly added plugins, the list has to be updated.

Signed-off-by: Arnaud Patard <apatard@hupstream.com>
@konstruktoid @apatard @dankow
Perform linting on Ansible playbooks (ansible-community#101)
d8e5d03 
* skip experimental rules

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>

* Lint ansible files

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>

* fix missed alignment

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>

* Fix blk100 flake8 errors

Flake8 is giving errors about some empty lines in the code, so
remove the line to fix these errors.

Signed-off-by: Arnaud Patard <apatard@hupstream.com>

---------

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
Signed-off-by: Arnaud Patard <apatard@hupstream.com>
Co-authored-by: Arnaud Patard <apatard@hupstream.com>
@pre-commit-ci @dankow
[pre-commit.ci] pre-commit autoupdate (ansible-community#93)
b42c9c3 
updates:
- [github.com/psf/black: 22.12.0 → 23.1.0](psf/black@22.12.0...23.1.0)
- [github.com/ansible/ansible-lint.git: v6.10.2 → v6.14.2](https://github.com/ansible/ansible-lint.git/compare/v6.10.2...v6.14.2)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@konstruktoid @dankow
install python3 only when ansible_system == Linux (ansible-community#110
30739e6 
)

* assume windows when ansible_connection is winrm

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>

* replace raw: with package:

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>

* revert back to raw

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>

* use uname to check if it's a Linux system

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>

---------

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@ssbarnea @dankow
Make selinux an optional extra (ansible-community#89)
76039b7 
As our special selinux shim dependency also caused problems for
people that did not really need selinux, we now make it a simple
extra.
@dankow
Trigger checks
cad6f72
@dankow
Merge branch 'main' into support_platform_option
a684592
@dankow
Copy link
Contributor Author

dankow commented Apr 5, 2023

@ssbarnea - now that the checks all pass, are there any barriers to merging this PR?

@ssbarnea ssbarnea merged commit ff07615 into ansible-community:main Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssbarnea ssbarnea ssbarnea approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dankow @ThomasSteinbach @ssbarnea @apatard @zhan9san @konstruktoid