opensearch-project / ansible-playbook

🤖 A community repository for Ansible Playbook of OpenSearch Project.
https://opensearch.org/
Apache License 2.0
86 stars 96 forks source link

Fix a couple of ansible-lint errors #126

Closed mpsOxygen closed 1 year ago

mpsOxygen commented 1 year ago

Description

Fixes a couple of ansible-lint errors

Issues Resolved

Cosmetic changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

prudhvigodithi commented 1 year ago

Hey @mpsOxygen, thanks for the contribution, can you please fix the DCO? Thank you

mpsOxygen commented 1 year ago

Hey @mpsOxygen, thanks for the contribution, can you please fix the DCO? Thank you

Fixed.

prudhvigodithi commented 1 year ago

Hey @mpsOxygen by default Ansible infers the modules and plugins contained in ansible-core, so should we still use as Ansible.Builtin? @peterzhuamazon

peterzhuamazon commented 1 year ago

Hey @mpsOxygen by default Ansible infers the modules and plugins contained in ansible-core, so should we still use as Ansible.Builtin? @peterzhuamazon

I think it is just to be clear it is buildin? So the linting will not warn? Think we should add a linter at this point, per my issue #58 on this repo.

mpsOxygen commented 1 year ago

Hey @mpsOxygen by default Ansible infers the modules and plugins contained in ansible-core, so should we still use as Ansible.Builtin? @peterzhuamazon

Linting asks for it and it is a best practice in case there are modules with the same name but different namespace.

peterzhuamazon commented 1 year ago

Hi @mpsOxygen We recently merged another PR and yours now have some conflicts, sorry about it. Could you kindly fix those conflicts?

Also @saravanan30erd @prudhvigodithi I am ok with this changes, let me know your thoughts here. I think this is a good opportunity for us to introduce a linter workflow after this PR is merged.

Thanks

mpsOxygen commented 1 year ago

Ive changed the conflicting lines from pause module to wait_for module and also formatted it to be in yaml format instead of ini format ( ini format is inline with = instead of : )

- name: Wait for opensearch to startup
<<<<<<< main
  ansible.builtin.wait_for:
    host: "{{ hostvars[inventory_hostname]['ip'] }}"
    port: "{{ os_api_port }}"
    delay: 5
    connect_timeout: 1
    timeout: 120
=======
  ansible.builtin.wait_for: host={{ hostvars[inventory_hostname]['ip'] }} port={{os_api_port}} delay=5 connect_timeout=1 timeout=120

>>>>>>> main
peterzhuamazon commented 1 year ago

Hi @saravanan30erd @prudhvigodithi could you have you thoughts on this? So we can move ahead with the changes. Thanks.

saravanan30erd commented 1 year ago

@peterzhuamazon It looks fine to me. Ansible recommends to use builtin for easy linking to the module documentation and to avoid conflicting with other collections that may have the same module name.

prudhvigodithi commented 1 year ago

Hey @mpsOxygen can you please fix the conflicts and we can merge this PR?

mpsOxygen commented 1 year ago

Hey @mpsOxygen can you please fix the conflicts and we can merge this PR?

The conflict is fixed in my opinion, it's just a difference in formating from ini format to yaml format.

prudhvigodithi commented 1 year ago

The Github reports as This branch has conflicts that must be resolved, please check @mpsOxygen Screenshot 2023-04-07 at 8 19 13 AM

mpsOxygen commented 1 year ago

The Github reports as This branch has conflicts that must be resolved, please check @mpsOxygen Screenshot 2023-04-07 at 8 19 13 AM

I have checked and the conflict is just a difference in formatting:

- name: Wait for opensearch to startup
<<<<<<< main
  ansible.builtin.wait_for:
    host: "{{ hostvars[inventory_hostname]['ip'] }}"
    port: "{{ os_api_port }}"
    delay: 5
    connect_timeout: 1
    timeout: 120
=======
  ansible.builtin.wait_for: host={{ hostvars[inventory_hostname]['ip'] }} port={{os_api_port}} delay=5 connect_timeout=1 timeout=120

>>>>>>> main
saravanan30erd commented 1 year ago

@mpsOxygen we cannot merge this PR unless this conflict is resolved. Please do git rebase and resolve the conflicts. Refer: https://docs.github.com/en/get-started/using-git/resolving-merge-conflicts-after-a-git-rebase

mpsOxygen commented 1 year ago

Ok, now I get it. Someone changed the file after my initial PR. But the rebase instructions for solving the conflict don't work because I have no rebase in my local repo as it is based on my PR. Could you give me some more info on how to solve this please?

prudhvigodithi commented 1 year ago

Hey @mpsOxygen can you please try with https://stackoverflow.com/questions/3903817/pull-new-updates-from-original-github-repository-into-forked-github-repository?

mpsOxygen commented 1 year ago

I think the conflicts are resolved now. Could you please check?

prudhvigodithi commented 1 year ago

Thanks @saravanan30erd @peterzhuamazon can you please add your approval? Thank you