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

Make user_medias optional/override #261

Closed
elrondvega opened this issue Nov 13, 2020 · 7 comments · Fixed by #264
Closed

Make user_medias optional/override #261

elrondvega opened this issue Nov 13, 2020 · 7 comments · Fixed by #264
Labels
module The issue or pull request is related to Zabbix module

Comments

@elrondvega
Copy link
Contributor

SUMMARY

Currently with the way user_medias works it overrides anything the end-user may have manually configured, even when user_medias isn't set, its defaulting to an empty array which is removing any configuration the user may have done manually. Per the API documentation the user_medias field is optional.

Don't update the user_medias when not set and/or provide an override setting like passwd has.

ISSUE TYPE
  • Feature Idea
COMPONENT NAME

zabbix_user

ADDITIONAL INFORMATION
@sky-joker
Copy link
Contributor

sky-joker commented Nov 13, 2020

Thank you @elrondvega for the feature idea.

The zabbix_user module isn’t currently an assumption of manual operation.
Because the configuration management and operation and playbook implement complex if assuming that allowing manual operation.
So if automation does, the configuration management ideal considering should use CMDB or YAML(host_vars and so on).
By managing the configuration in YAML with GitLab or GitHub, change history is automatically recorded and the latest settings can be found in YAML.
In this way, there will be no difference in the configuration between the real machine and YAML.
This is the best way to do it, assuming an automated operation.

But, I understand your feeling.
If everyone wants this feature, I will consider implementing it.
I just need to keep an eye on this for a while.

@sky-joker sky-joker added the module The issue or pull request is related to Zabbix module label Nov 13, 2020
@D3DeFi
Copy link
Contributor

D3DeFi commented Nov 13, 2020

This closely relates to what I have been observing with zabbix_host and other modules. Instead of defaulting to empty iterable that is not even required by Zabbix API we should default to None. This would give us freedom that we don't need to care about such parameters, just ignore everything from module.params that has such value.

This will ofc only work when we remove any parameters with None value from zbxobj.create/update calls. For example I consider this a bug in zabbix_host <- we should not need force=no there (although module design prevents it removal now) and I would like to prevent it being implemented elsewhere.

@elrondvega
Copy link
Contributor Author

The zabbix_user module isn’t currently an assumption of manual operation.

Sorry maybe I need to clarify, the module shouldn't assume just because something isn't set that it should be erased.

In this way, there will be no difference in the configuration between the real machine and YAML.

Ansible should only manage and configure the things it has been configured to manage, it shouldn't assume just because something isn't set/configured, that is should just remove those settings.

@sky-joker
Copy link
Contributor

This closely relates to what I have been observing with zabbix_host and other modules. Instead of defaulting to empty iterable that is not even required by Zabbix API we should default to None.

Unfortunately, an error occurred that I tried after I change the user_media default value from list to None.
(I'm sorry if I don't understand correctly.)
The user_medias seems necessary to be the list(array) type.

https://www.zabbix.com/documentation/5.0/manual/api/reference/user/create

If implementation of this request feature to the module, it needs to get the existing user_media configures from Zabbix Server and include the configures to request_data when updating a Zabbix user.

@elrondvega
Copy link
Contributor Author

I was able to implement this change without querying the current state as the field is optional. I will submit a pull request.

@D3DeFi
Copy link
Contributor

D3DeFi commented Nov 16, 2020

I was able to implement this change without querying the current state as the field is optional. I will submit a pull request.

Exactly. I meant my previous comment for all options that are not required. We should default those to None and then remove every None from the request data before sending create/update API calls

@sky-joker
Copy link
Contributor

I see, That's what you mean.
I get it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module The issue or pull request is related to Zabbix module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants