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

docker_swarm_service: Documentation fixes #50861

Merged

Conversation

hannseman
Copy link
Contributor

@hannseman hannseman commented Jan 14, 2019

SUMMARY

This PR fixes three issues in the documentation.

  • The minimal docker API version is not documented for secrets and configs.
  • labels and container_labels are described as they accept lists but in reality they expect dicts.
  • limit_memory and reserve_memory describes that they accept values in "MB" but in reality it is in bytes if not suffixed by unit identifiers.
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

docker_swarm_service

@ansibot
Copy link
Contributor

ansibot commented Jan 14, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. cloud community_review In order to be merged, this PR must follow the community review workflow. docker module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:community This issue/PR relates to code supported by the Ansible community. labels Jan 14, 2019
@ansibot
Copy link
Contributor

ansibot commented Jan 14, 2019

The test ansible-test sanity --test pep8 [explain] failed with 4 errors:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:145:69: W291 trailing whitespace
lib/ansible/modules/cloud/docker/docker_swarm_service.py:146:71: W291 trailing whitespace
lib/ansible/modules/cloud/docker/docker_swarm_service.py:153:75: W291 trailing whitespace
lib/ansible/modules/cloud/docker/docker_swarm_service.py:154:71: W291 trailing whitespace

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jan 14, 2019
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. new_contributor This PR is the first contribution by a new community member. labels Jan 14, 2019
@hannseman
Copy link
Contributor Author

ready_for_review

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 14, 2019
@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jan 14, 2019
@felixfontein
Copy link
Contributor

Anyway, you might also be interested in the Ansible Docker WG; see ansible/community#408 for some infos. Feel free to post ideas there or just subscribe to it to find out what's going on.

Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

LGTM shipit

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jan 14, 2019
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

LGTM shipit

@gundalow gundalow merged commit 644057e into ansible:devel Jan 14, 2019
@gundalow
Copy link
Contributor

@hannseman Thank you for this improvement
@felixfontein Thanks for the review!

felixfontein pushed a commit to felixfontein/ansible that referenced this pull request Jan 14, 2019
* Describe labels and container_labels correctly

* Clarify reserve_memory and limit_memory docs

* Remove default from container_labels doc

* Remove trailing whitespace

* Document min api version for configs and secrets

* Add changelog fragment

* Specify type on labels and container_labels

* Consolidate required API version descriptions

* Update reserve and limit memory docs

* Use correct power-of-two units

* Remove description about limit_memory minimum 4mb

(cherry picked from commit 644057e)
@felixfontein
Copy link
Contributor

Thanks everyone! I created a backport PR in #50873.

abadger pushed a commit that referenced this pull request Jan 14, 2019
* Describe labels and container_labels correctly

* Clarify reserve_memory and limit_memory docs

* Remove default from container_labels doc

* Remove trailing whitespace

* Document min api version for configs and secrets

* Add changelog fragment

* Specify type on labels and container_labels

* Consolidate required API version descriptions

* Update reserve and limit memory docs

* Use correct power-of-two units

* Remove description about limit_memory minimum 4mb

(cherry picked from commit 644057e)
knumskull pushed a commit to knumskull/ansible that referenced this pull request Jan 21, 2019
* Describe labels and container_labels correctly

* Clarify reserve_memory and limit_memory docs

* Remove default from container_labels doc

* Remove trailing whitespace

* Document min api version for configs and secrets

* Add changelog fragment

* Specify type on labels and container_labels

* Consolidate required API version descriptions

* Update reserve and limit memory docs

* Use correct power-of-two units

* Remove description about limit_memory minimum 4mb
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. cloud docker module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants