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

Apply the same logic of renaming parameters #495

Merged

Conversation

klando
Copy link
Contributor

@klando klando commented Oct 22, 2021

SUMMARY

I renamed some more parameters used when working with API.

ISSUE TYPE
  • Improvment Pull Request
COMPONENT NAME

roles zabbix_agent and zabbix_proxy

ADDITIONAL INFORMATION

Working more with the collection, I wonder why zabbix_agent role is not named zabbix_host... It looks like it's based on the packages name, but using the concepts of the webUI (host instead of agent) might be interesting.

@dj-wasabi
Copy link
Contributor

It follows the name from Zabbix themself. They talk about Zabbix Server, Agents and Proxies. So thats why zabbix_agent (and thus variables) are named with an _agent. :)

@klando
Copy link
Contributor Author

klando commented Oct 22, 2021

It follows the name from Zabbix themself. They talk about Zabbix Server, Agents and Proxies. So thats why zabbix_agent (and thus variables) are named with an _agent. :)

I tend to agree. Note that in zabbix there are processes and configurations, one matches well with collection modules, the other one with roles maybe. I don't want to argue here, I'm not convinced myself yet :)

@klando
Copy link
Contributor Author

klando commented Oct 22, 2021

It follows the name from Zabbix themself. They talk about Zabbix Server, Agents and Proxies. So thats why zabbix_agent (and thus variables) are named with an _agent. :)

I tend to agree. Note that in zabbix there are processes and configurations, one matches well with collection modules, the other one with roles maybe. I don't want to argue here, I'm not convinced myself yet :)

Re-reading your comment, just to make sure: you agree with the PR ?

@dj-wasabi
Copy link
Contributor

It follows the name from Zabbix themself. They talk about Zabbix Server, Agents and Proxies. So thats why zabbix_agent (and thus variables) are named with an _agent. :)

I tend to agree. Note that in zabbix there are processes and configurations, one matches well with collection modules, the other one with roles maybe. I don't want to argue here, I'm not convinced myself yet :)

Re-reading your comment, just to make sure: you agree with the PR ?

I would like to keep the _agent in the name, to follow the name that Zabbix uses as well. Otherwise people might get confused.

@klando
Copy link
Contributor Author

klando commented Oct 22, 2021

It follows the name from Zabbix themself. They talk about Zabbix Server, Agents and Proxies. So thats why zabbix_agent (and thus variables) are named with an _agent. :)

I tend to agree. Note that in zabbix there are processes and configurations, one matches well with collection modules, the other one with roles maybe. I don't want to argue here, I'm not convinced myself yet :)

Re-reading your comment, just to make sure: you agree with the PR ?

I would like to keep the _agent in the name, to follow the name that Zabbix uses as well. Otherwise people might get confused.

I've kept original parameters so users should not be confused.
As for changing, if you look at the UPGRADE file, it outlines that the zabbix_agent was not used every time.

Can you elaborate on what might lead users to confusion ? Be aware that as a user of the collection I myself was confused by those parameters name vs usage thus the PRs...

@klando klando force-pushed the for-upstream-more-vars-review branch from e50f86b to da3368b Compare October 22, 2021 15:34
@dj-wasabi
Copy link
Contributor

It is the Zabbix Agent role, so parameters should contain or start withzabbix_agent_ and not with zabbix_host_. It will be confusing to install the Zabbix Agent with zabbix_host_ parameters.

@klando
Copy link
Contributor Author

klando commented Oct 22, 2021

It is the Zabbix Agent role, so parameters should contain or start withzabbix_agent_ and not with zabbix_host_. It will be confusing to install the Zabbix Agent with zabbix_host_ parameters.

I believe we are not talking of the same thing. My comment in Additionnal information was not about things done in "this" PR.
This PR does the following. Note how zabbix_agent_ was not really used as a prefix.... And let me know what is going to be more confusing than before :

From To
zabbix_server_url zabbix_api_server_url
zabbix_http_user zabbix_api_http_user
zabbix_http_password zabbix_api_http_password
zabbix_login_user zabbix_api_login_user
zabbix_login_pass zabbix_api_login_pass
zabbix_validate_certs zabbix_api_validate_certs
zabbix_create_hostgroup zabbix_hostgroups_state
zabbix_macros zabbix_hostmacros
zabbix_agent_description zabbix_host_description
zabbix_agent_interfaces zabbix_host_interfaces
zabbix_inventory_mode zabbix_host_inventory_mode
zabbix_agent_inventory_zabbix zabbix_host_inventory_zabbix
zabbix_link_templates zabbix_host_link_templates
zabbix_proxy zabbix_host_proxy
zabbix_update_host zabbix_host_update
zabbix_create_host zabbix_host_state
zabbix_visible_hostname zabbix_host_visible_hostname

And

From To
zabbix_server_host zabbix_proxy_server
zabbix_server_port zabbix_proxy_serverport
zabbix_proxy_localbuffer zabbix_proxy_proxylocalbuffer
zabbix_proxy_offlinebuffer zabbix_proxy_proxyofflinebuffer
zabbix_create_proxy zabbix_proxy_state
zabbix_server_url zabbix_api_server_url
zabbix_http_user zabbix_api_http_user
zabbix_http_password zabbix_api_http_password
zabbix_login_user zabbix_api_login_user
zabbix_login_pass zabbix_api_login_pass
zabbix_validate_certs zabbix_api_validate_certs

@klando klando force-pushed the for-upstream-more-vars-review branch from da3368b to 81572fd Compare October 22, 2021 17:50
@dj-wasabi
Copy link
Contributor

I was talking about this part, I would not expect the "to" starting with zabbix_host_, but with zabbix_agent_, like the following:

From To
zabbix_create_hostgroup zabbix_agent_hostgroups_state
zabbix_macros zabbix_agent_macros
zabbix_agent_description zabbix_agent_description
zabbix_agent_interfaces zabbix_agent_interfaces
zabbix_inventory_mode zabbix_agent_inventory_mode
zabbix_agent_inventory_zabbix zabbix_agent_inventory_zabbix
zabbix_link_templates zabbix_agent_link_templates
zabbix_proxy zabbix_agent_proxy
zabbix_update_host zabbix_agent_host_update
zabbix_create_host zabbix_agent_host_state
zabbix_visible_hostname zabbix_agent_visible_hostname

The rest is all fine by me, because that makes actual sense.

@klando
Copy link
Contributor Author

klando commented Oct 22, 2021

I was talking about this part, I would not expect the "to" starting with zabbix_host_, but with zabbix_agent_, like the following:
From To
zabbix_create_hostgroup zabbix_agent_hostgroups_state
zabbix_macros zabbix_agent_macros
zabbix_agent_description zabbix_agent_description
zabbix_agent_interfaces zabbix_agent_interfaces
zabbix_inventory_mode zabbix_agent_inventory_mode
zabbix_agent_inventory_zabbix zabbix_agent_inventory_zabbix
zabbix_link_templates zabbix_agent_link_templates
zabbix_proxy zabbix_agent_proxy
zabbix_update_host zabbix_agent_host_update
zabbix_create_host zabbix_agent_host_state
zabbix_visible_hostname zabbix_agent_visible_hostname

The rest is all fine by me, because that makes actual sense.

I see. I see your point.
I will still argue with an example: in my setup I configure hostgroups during the deployment of zabbix-web and I do not expect proxies or agents to create hostgroups. Thus I wanted to propose a new feature for the role zabbix_web, doing:

# tasks file for zabbix_manager
- name: Configure Zabbix Server
  block:
  - name: Ensure relevant host groups exist
    community.zabbix.zabbix_group:
      host_groups: "{{ zabbix_host_groups }}"
      login_password: "{{ zabbix_api_login_pass }}"
      login_user: "{{ zabbix_api_login_user }}"
      server_url: "{{ zabbix_api_server_url }}"
      state: present

zabbix_host_groups is the same as proposed for role zabbix_agent, it is very convenient and I believe outline why the name proposed is relevant, We can have both also, but it's a bit spaghetti plate...

@dj-wasabi
Copy link
Contributor

Yes, I added it in the Agent because I can not assume people have the Zabbix Web deployed (via my role) and people might just use the Zabbix Agent role and nothing else. (Not even sure if I had back then already the zabbix-web role). So even these people should be - when they want to - be able to configure some basic things like a hostgroup to be present when they add a new agent.

If you think it also makes sense to add that logic to the Zabbix Web role, fine by me and I happly merge the PR. But I think it is important that the Agent role also has and keep that functionality (I would either expect an issue to arise or a new PR to add it when it will be removed).

@klando
Copy link
Contributor Author

klando commented Oct 25, 2021

Yes, I added it in the Agent because I can not assume people have the Zabbix Web deployed (via my role) and people might just use the Zabbix Agent role and nothing else. (Not even sure if I had back then already the zabbix-web role). So even these people should be - when they want to - be able to configure some basic things like a hostgroup to be present when they add a new agent.

If you think it also makes sense to add that logic to the Zabbix Web role, fine by me and I happly merge the PR. But I think it is important that the Agent role also has and keep that functionality (I would either expect an issue to arise or a new PR to add it when it will be removed).

To make it clear, I have applied your suggestions. I may be back on this topic later though, as I still find the solution confusing to end users (zabbix agent is a software deployed with related configuration file, but then zabbix agent is only a component of a zabbix host, and IMHO the role should have been zabbix_host). Now the boat has shipped, and changes may happen on major upgrade, they exists precisely for that purpose.

@klando
Copy link
Contributor Author

klando commented Oct 25, 2021

During rebase I noticed some stuff I overlooked. Will fix and update PR.

@dj-wasabi
Copy link
Contributor

The names of the roles follows the naming of the Zabbix component which can be found here: https://www.zabbix.com/documentation/current/manual/introduction/overview If someone wants to deploy a Server, the person should use the zabbix_server role, when deploying an Agent, it should deploy the zabbix_agent role. Renaming the role to zabbix_host (which is nowhere to be found in the documentation in Zabbix) is only confusing for those who read the official documentation of Zabbix and want to make use of Ansible to setup their environment.

@klando
Copy link
Contributor Author

klando commented Oct 25, 2021

The names of the roles follows the naming of the Zabbix component which can be found here: https://www.zabbix.com/documentation/current/manual/introduction/overview If someone wants to deploy a Server, the person should use the zabbix_server role, when deploying an Agent, it should deploy the zabbix_agent role. Renaming the role to zabbix_host (which is nowhere to be found in the documentation in Zabbix) is only confusing for those who read the official documentation of Zabbix and want to make use of Ansible to setup their environment.

We don't read the same doc:
https://www.zabbix.com/documentation/current/manual/config/hosts

@klando
Copy link
Contributor Author

klando commented Oct 25, 2021

The names of the roles follows the naming of the Zabbix component which can be found here: https://www.zabbix.com/documentation/current/manual/introduction/overview If someone wants to deploy a Server, the person should use the zabbix_server role, when deploying an Agent, it should deploy the zabbix_agent role. Renaming the role to zabbix_host (which is nowhere to be found in the documentation in Zabbix) is only confusing for those who read the official documentation of Zabbix and want to make use of Ansible to setup their environment.

We don't read the same doc: https://www.zabbix.com/documentation/current/manual/config/hosts

Precisely on the page you mentioned:

Data flow

In addition it is important to take a step back and have a look at the overall data flow within Zabbix. In order to create an item that gathers data you must first create a host. Moving to the other end of the Zabbix spectrum you must first have an item to create a trigger. You must have a trigger to create an action. Thus if you want to receive an alert that your CPU load is too high on Server X you must first create a host entry for Server X followed by an item for monitoring its CPU, then a trigger which activates if the CPU is too high, followed by an action which sends you an email. While that may seem like a lot of steps, with the use of templating it really isn't. However, due to this design it is possible to create a very flexible setup.

no agent but host. again.

@klando klando force-pushed the for-upstream-more-vars-review branch from f2a98f9 to f81778b Compare October 25, 2021 12:13
@klando
Copy link
Contributor Author

klando commented Oct 25, 2021

Hopefully all good here. I've rebased and it was a bit annoying because of the "lint pass"

@dj-wasabi please double check that no old variables disappeared, they are all supposed to get a comment like # Will be deprecated in 2.0.0

It's a bit fuzzy around zabbix_url/zabbix_server_url/zabbix_agent_server_url and all as usage was not consistent between roles and tests.

Also add zabbix_agent2_hostname

we were using only zabbix_agent_hostname for both agents before.
But it is also used in the template, so applied the same namming
convention.

Rename zabbix_create_hostgroup to zabbix_hostgroups_state

Try to apply some naming convention

Rename zabbix_link_templates to zabbix_host_link_templates

Same logic...

Rename zabbix_create_host to zabbix_host_state

Same logic

Rename zabbix_update_host to zabbix_host_update

Same logic....

Rename zabbix_proxy to zabbix_host_proxy

Same logic

Rename zabbix_inventory_mode to zabbix_host_inventory_mode

Same logic

Rename zabbix_agent_interfaces to zabbix_host_interfaces

Same logic

Rename zabbix_visible_hostname to zabbix_host_visible_hostname

Same logic

Rename zabbix_validate_certs to zabbix_api_validate_certs

Samez logic, for API

Rename zabbix_agent_description to zabbix_host_description

Same logic

Rename zabbix_agent_inventory_zabbix to zabbix_host_inventory_zabbix

Same logic

Rename zabbix_macros to zabbix_hostmacros

Same logic

Rename zabbix_create_proxy to zabbix_proxy_state

Same logic, and fix a leftover from previsou commit

Use zabbix_agent_ intsead of zabbix_host_

Per suggestion from @dj-wasabi
@klando klando force-pushed the for-upstream-more-vars-review branch from f81778b to 6805c01 Compare October 25, 2021 13:05
@klando
Copy link
Contributor Author

klando commented Oct 25, 2021

I've recheck and I believe you can merge if you agree with the changes.

@klando
Copy link
Contributor Author

klando commented Oct 26, 2021

@dj-wasabi while I'm arguing about the parameters name, my PR proposes what you validated including the rename with zabbix_agent_ prefix.
Is there any blocker for a merge ?

@klando klando mentioned this pull request Oct 28, 2021
@dj-wasabi dj-wasabi merged commit 836e60b into ansible-collections:main Oct 29, 2021
@dj-wasabi
Copy link
Contributor

Thanks! 👍

@klando klando deleted the for-upstream-more-vars-review branch October 30, 2021 10:21
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.

3 participants