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

Add: proxy_env variable #482

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

AliMehraji
Copy link

@AliMehraji AliMehraji commented Dec 8, 2024

Changes:

Add the proxy_env variable in:

  • defaults/main.yml

Add environment for proxy needed tasks in:

  • tasks/docker-compose.yml
  • tasks/main.yml
  • tasks/setup-Debian.yml
  • tasks/setup-RedHat.yml

@geerlingguy
Copy link
Owner

This seems like a lot of changes to support something that may be able to be set on a wider level instead of per-task...

@AliMehraji
Copy link
Author

AliMehraji commented Dec 9, 2024

This seems like a lot of changes to support something that may be able to be set on a wider level instead of per-task...

I agree that implementing this at the role level could offer a more streamlined approach. My initial intention was to apply it on a per-task basis to ensure it's only activated when specifically needed, thereby providing more granular control and avoiding unintended side effects across all tasks within the role.

However, I'm open to exploring a role-level configuration or any recommendation you might have. I’d appreciate your guidance on the best approach to align with our overall strategy and maintainability goals.

For instance:

The proxy environment is not needed in the Ensure old versions of Docker are not installed. task within tasks/setup-RedHat.yml, which is why I implemented it on a per-task basis.

@AliMehraji
Copy link
Author

AliMehraji commented Dec 17, 2024

Additionally:

  • Plan to update module references to use fully qualified collection names.
  • Resolve other ansible-lint and yamllint warnings.

Do you have any recommendations or preferences for these changes?

Example:

  • In handlers/main.yml, the service module would be updated to ansible.builtin.service
  • In tasks/docker-users.yml, the user module would be updated to ansible.builtin.user

@AliMehraji
Copy link
Author

@geerlingguy (Happy New Year :) )

CHANGES

Modified

.ansible-lint

  • add start of yaml file --- (Line 1)
  • add fqcn-builtins in enable_list (Lines 7-8)

.github/workflows/ci.yml

  • changed dependencies installation from file named requirements.txt (Line 63)

.yamllint

  • line lenght (from 200 to 240) (Line 12)
  • add yaml-files (Lines 4-8)
  • add ansible-lint recommendations (Lines 14-21)
    • anisble-recommendation:

      WARNING  Found incompatible custom yamllint configuration (.yamllint), please either remove the file or edit it to comply with:
      - comments.min-spaces-from-content must be 1
      - comments-indentation must be false
      - braces.max-spaces-inside must be 1
      - octal-values.forbid-implicit-octal must be true
      - octal-values.forbid-explicit-octal must be true.
      

defaults/main.yml

  • rename: docker_obsolete_packages --> docker_obsolete_packages_debian , also add docker-compose and docker-compose-v2 (lines 35,36)
  • Add docker_obsolete_packages_redhat variable (According to docker engin installation document)
  • changed "v2.29.2" to "v2.32.1" for docker_compose_version variable
  • add spaces in lines 91 and 94 (ansible-lint recommendation)

handlers/main.yml

  • name: restart docker --> name: Restart docker (ansible-lint: All names should start with an uppercase letter.)
  • service --> ansible.builtin.service (used FQCN)

meta/main.yml

  • min_ansible_version: 2.10 --> "2.10" (ansible-lint: galaxy_info.min_ansible_version 2.1 is not of type 'string')

molecule/default/converge.yml

  • apt: update_cache=yes cache_valid_time=600 --> ansible.builtin.apt (Line 8, ansible-lint: Use FQCN for apt module and Avoid using free-form when calling module actions)
  • # noqa 303 --> # noqa command-instead-of-module (Line 13 , ansible-lint: Replaced outdated tag '303' with 'command-instead-of-module')
  • command --> ansible.builtin.command (Line 14, ansible-lint: Use FQCN for command module)

molecule/default/molecule.yml

  • Add requirements-file: requirements.yml (Line 7)

docker-compose.yml --> setup-docker-compose.yml

  • command --> ansible.builtin.command (Line 3)
  • add name for set_fact task (Line 9) also set_fact --> ansible.builtin.set_fact (Line 10)
  • file --> ansible.builtin.file (Line 17)
  • get_url --> ansible.builtin.get_url (Line 25)
  • mode: 0755 --> mode: "0755" (Line 28)

tasks/docker-users.yml

  • user --> ansible.builtin.user (Line 3, ansible-lint: Use FQCN for user module )
  • meta --> ansible.builtin.meta (Line 10, ansible-lint: Use FQCN for user module )

tasks/main.yml

  • changes:
    • add more space arround the braces in lines 7 and 8 '{{ ansible_distribution }}.yml'
    • all modules changed to FQCN format
    • notify handlers name changed to uppercase letter according to its changed in handlers/main.yml
    • add double-quote for all file modes , for instance in Configure Docker daemon options. task
    • add # noqa: name[missing] for include tasks (avoiding redundancy for naming everything, also ignorring ansible-lint all tasks should have name)
    • include_tasks: docker-compose.yml --> - include_tasks: setup-docker-compose.yml (because of the rename)

README.md

  • docker_compose_version changed from "2.29.2" to v2.32.1 currently latest docker compose version (Line 79)
  • add docker_obsolete_packages_debian (Lines 33-42)
  • add docker_obsolete_packages_redhat (Lines 45-55)

new files

  • requirements.txt
  • requirements.yml

renamed files

  • docker-compose.yml --> setup-docker-compose.yml

…_packages_redhat variable in defaults/main.yml
@AliMehraji
Copy link
Author

Hello @geerlingguy Again

Could you please consider the PR? Review it, and let me know if there is any problem so I can fix it.

@geerlingguy
Copy link
Owner

This is a lot of changes, practically rewriting the entire role! I don't generally set any proxy settings in any of my roles, so I would rather not add that change. The other changes may be helpful, but there are so many in one PR, it is hard for me to review them.

@AliMehraji
Copy link
Author

This is a lot of changes, practically rewriting the entire role! I don't generally set any proxy settings in any of my roles, so I would rather not add that change. The other changes may be helpful, but there are so many in one PR, it is hard for me to review them.

Thank you for your feedback!

I understand that this PR includes many changes, which can make it challenging to review. To make the review process easier, I’ll split the changes into multiple, smaller pull requests.

Regarding the proxy settings:

I completely understand your preference to avoid including proxy configurations in roles. However, I’d like to point out that not everyone can access a mirror or private registry to
set it in daemon.json. Additionally, some users may need to rely on a proxy to make this role functional in their environments due to sanctions, filtering, or network restrictions.

By keeping the proxy configuration disabled by default, it would not impact users who don’t need it while providing flexibility for those who do.

I’m happy to discuss this further if you have any concerns!

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