dconf: three minor but useful fixes #6206
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jikamens
added a commit
to jikamens/community.general
that referenced
this pull request
Mar 19, 2023
ansibullbot
added
bug
This comment was marked as outdated.
This comment was marked as outdated.
ansibullbot
added
ci_verified
jikamens
added a commit
to jikamens/community.general
that referenced
this pull request
Mar 19, 2023
ansibullbot
added
ci_verified
|
One of the tests here is failing because the documentation says that the value argument is supposed to be str but the code says it's supposed to be raw. That discrepancy is an intentional component of this fix: the argument should be documented as a string argument, but internally it needs to be raw so I can fix booleans. |
felixfontein
added
check-before-release
plugins/modules/dconf.py
Outdated
| if isinstance(module.params['value'], bool): | ||
| module.params['value'] = 'true' if module.params['value'] else 'false' | ||
| else: | ||
| module.params['value'] = str(module.params['value']) |
There was a problem hiding this comment.
Please use
Suggested change
| module.params['value'] = str(module.params['value']) | |
| module.params['value'] = to_native(module.params['value'], errors='surrogate_or_strict') |
instead. This is what AnsibleModule will do when type='str' is in the argument spec.
(You need from ansible.module_utils.common.text.converters import to_native for this if it isn't there already.)
|
Made requested changes, please take a look and let me know what you think. |
This comment was marked as outdated.
This comment was marked as outdated.
ansibullbot
added
ci_verified
We need to check if the value in the database is None before we try to parse it, because the GVariant parser won't accept None as an input value. By definition if the value is None, i.e., there's no value in the database, than any value the user is trying to set is a change, so just indicate that it's a change without trying to compare the None to whatever the user specified as the value.x
if writing a key fails, then include in the error that is returned the exact key and value aguments that were given to the dconf command, to assist in diagnosing failures caused by providing the key or value in the wrong format.x
Even though we warn users to be careful to specify GVariant strings for values, a common error is to be trying to specify a boolean string which ends up getting converted into a boolean by the YAML parser or Ansible. Then it gets converted to "True" or "False", the string representations of Python booleans, which are not valid GVariants. Rather than just failing with an obscure error when this happens, let's be more user-friendly and detect when the user has specified a boolean and convert it into the correct GVariant forms, "true" or "false", so it just works. There's no good reason to be more pedantic than that.
|
Thanks. I rebased down to three distinct commits for cleanliness before the merge. |
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
|
Please note that we squash-merge anyway :) |
Hmm, I hope that if the PR is already cleaned up and has commits which serve distinct purposes, you don't squash those commits when merging? |
1 task
|
Well, we always squash-merge. If you want different commits, you need to create multiple PRs. |
Backport to stable-6: 💚 backport PR created✅ Backport PR branch: Backported as #6330 🤖 @patchback |
felixfontein
removed
the
check-before-release
patchback bot
pushed a commit
that referenced
this pull request
Apr 13, 2023
* dconf: Correctly handle setting a key that has no value in DB We need to check if the value in the database is None before we try to parse it, because the GVariant parser won't accept None as an input value. By definition if the value is None, i.e., there's no value in the database, than any value the user is trying to set is a change, so just indicate that it's a change without trying to compare the None to whatever the user specified as the value.x * dconf: Give a more useful error when writing a key fails if writing a key fails, then include in the error that is returned the exact key and value aguments that were given to the dconf command, to assist in diagnosing failures caused by providing the key or value in the wrong format.x * dconf: Convert boolean values into the format that dconf expects Even though we warn users to be careful to specify GVariant strings for values, a common error is to be trying to specify a boolean string which ends up getting converted into a boolean by the YAML parser or Ansible. Then it gets converted to "True" or "False", the string representations of Python booleans, which are not valid GVariants. Rather than just failing with an obscure error when this happens, let's be more user-friendly and detect when the user has specified a boolean and convert it into the correct GVariant forms, "true" or "false", so it just works. There's no good reason to be more pedantic than that. (cherry picked from commit a576514)
|
@jikamens thanks a lot for your contribution! |
felixfontein
pushed a commit
that referenced
this pull request
Apr 13, 2023
…fixes (#6330) dconf: three minor but useful fixes (#6206) * dconf: Correctly handle setting a key that has no value in DB We need to check if the value in the database is None before we try to parse it, because the GVariant parser won't accept None as an input value. By definition if the value is None, i.e., there's no value in the database, than any value the user is trying to set is a change, so just indicate that it's a change without trying to compare the None to whatever the user specified as the value.x * dconf: Give a more useful error when writing a key fails if writing a key fails, then include in the error that is returned the exact key and value aguments that were given to the dconf command, to assist in diagnosing failures caused by providing the key or value in the wrong format.x * dconf: Convert boolean values into the format that dconf expects Even though we warn users to be careful to specify GVariant strings for values, a common error is to be trying to specify a boolean string which ends up getting converted into a boolean by the YAML parser or Ansible. Then it gets converted to "True" or "False", the string representations of Python booleans, which are not valid GVariants. Rather than just failing with an obscure error when this happens, let's be more user-friendly and detect when the user has specified a boolean and convert it into the correct GVariant forms, "true" or "false", so it just works. There's no good reason to be more pedantic than that. (cherry picked from commit a576514) Co-authored-by: Jonathan Kamens <jik@kamens.us>
1 task
netbsd-srcmastr
pushed a commit
to NetBSD/pkgsrc
that referenced
this pull request
Apr 26, 2023
v7.5.0 Minor Changes ------------- ansible.posix ~~~~~~~~~~~~~ - Add jsonl callback plugin to ansible.posix collection - firewalld - add `protocol` parameter cisco.aci ~~~~~~~~~ - Add aci_access_span_dst_group module for fabric access policies span destination group support - Add aci_access_span_filter_group and aci_access_span_filter_group_entry modules for access span filter group support - Add aci_config_export_policy module - Add aci_igmp_interface_policy module - Add aci_interface_config module for new interface configuration available in ACI v5.2(5)+ - Add aci_interface_policy_spanning_tree module cisco.dnac ~~~~~~~~~~ - accesspoint_configuration_details_by_task_id_info - new module - authentication_policy_servers_info - new module - credential_to_site_by_siteid_create_v2 - new module - device_interface_info - attributes `lastInputTime` and `lastOutputTime` were added. - device_reboot_apreboot_info - new module - dnac_packages_info - new module - eox_status_device_info - new module - eox_status_summary_info - new module - event_email_config - new module - event_email_config_info - new module - event_snmp_config_info - new module - event_syslog_config - new module - event_syslog_config_info - new module - execute_suggested_actions_commands - new module - global_credential_v2 - new module - global_credential_v2_info - new module - integration_settings_instances_itsm - new module - integration_settings_instances_itsm_info - new module - lan_automation_log_by_serial_number_info - new module - network_device_user_defined_field - new module - network_device_user_defined_field_info - new module - network_v2 - new module - network_v2_info - new module - pnp_device_claim_to_site - attributes `removeInactive` and `hostname` were removed. - role_permissions_info - new module - roles_info - new module - sda_fabric_border_device - attributes `routeDistributionProtocol` and `borderPriority` were added. - sda_fabric_control_plane_device attribute `routeDistributionProtocol` was added. - sda_fabric_edge_device - attribute `siteNameHierarchy` was added. - sda_fabric_site - attribute `fabricType` was added. - sda_port_assignment_for_user_device - attribute `interfaceNames` was added. - sda_virtual_network - attribute `vManageVpnId` was added. - sda_virtual_network_ip_pool - attribute `isBridgeModeVm` was added. - sda_virtual_network_v2 - attribute `isBridgeModeVm` was added. - service_provider_v2 - new module - service_provider_v2_info - new module - sp_profile_delete_v2 - new module - user - new module - user_info - new module - users_external_servers_info - new module - wireless_accespoint_configuration - new module - wireless_accesspoint_configuration_summary_info - new module cisco.ios ~~~~~~~~~ - ios_bgp_address_family - add option redistribute.ospf.include_connected when redistributing OSPF in IPv6 AFI - ios_bgp_address_family - add option redistribute.ospf.match.externals.type_1 to allow - ios_bgp_address_family - add option redistribute.ospf.match.externals.type_2 to allow - specification of OSPF E1 routes - specification of OSPF E2 routes cisco.mso ~~~~~~~~~ - Add ip_data_plane_learning and preferred_group arguments to mso_schema_template_vrf module - Add module mso_schema_site_anp_epg_bulk_staticport - Add route_reachability attribute to mso_schema_site_external_epg module cisco.nxos ~~~~~~~~~~ - `nxos_route_maps` - add support for 'set ip next-hop <>' command in route-maps - `nxos_vxlan_vtep` - add support for 'advertise virtual-rmac' command under nve interface community.crypto ~~~~~~~~~~~~~~~~ - get_certificate - add ``asn1_base64`` option to control whether the ASN.1 included in the ``extensions`` return value is binary data or Base64 encoded (ansible-collections/community.crypto#592). community.general ~~~~~~~~~~~~~~~~~ - cpanm - minor change, use feature from ``ModuleHelper`` (ansible-collections/community.general#6385). - dconf - be forgiving about boolean values: convert them to GVariant booleans automatically (ansible-collections/community.general#6206). - dconf - minor refactoring improving parameters and dependencies validation (ansible-collections/community.general#6336). - deps module utils - add function ``failed()`` providing the ability to check the dependency check result without triggering an exception (ansible-collections/community.general#6383). - dig lookup plugin - Support multiple domains to be queried as indicated in docs (ansible-collections/community.general#6334). - gitlab_project - add new option ``topics`` for adding topics to GitLab projects (ansible-collections/community.general#6278). - homebrew_cask - allows passing ``--greedy`` option to ``upgrade_all`` (ansible-collections/community.general#6267). - idrac_redfish_command - add ``job_id`` to ``CreateBiosConfigJob`` response (ansible-collections/community.general#5603). - ipa_hostgroup - add ``append`` parameter for adding a new hosts to existing hostgroups without changing existing hostgroup members (ansible-collections/community.general#6203). - keycloak_authentication - add flow type option to sub flows to allow the creation of 'form-flow' sub flows like in Keycloak's built-in registration flow (ansible-collections/community.general#6318). - mksysb - improved the output of the module in case of errors (ansible-collections/community.general#6263). - nmap inventory plugin - added environment variables for configure ``address`` and ``exclude`` (ansible-collections/community.general#6351). - nmcli - add ``macvlan`` connection type (ansible-collections/community.general#6312). - pipx - add ``system_site_packages`` parameter to give application access to system-wide packages (ansible-collections/community.general#6308). - pipx - ensure ``include_injected`` parameter works with ``state=upgrade`` and ``state=latest`` (ansible-collections/community.general#6212). - puppet - add new options ``skip_tags`` to exclude certain tagged resources during a puppet agent or apply (ansible-collections/community.general#6293). - terraform - remove state file check condition and error block, because in the native implementation of terraform will not cause errors due to the non-existent file (ansible-collections/community.general#6296). - udm_dns_record - minor refactor to the code (ansible-collections/community.general#6382). community.zabbix ~~~~~~~~~~~~~~~~ - httpapi plugin - updated to work with Zabbix 6.4. - zabbix_action, zabbix_authentication, zabbix_discovery_rule, zabbix_mediatype, zabbix_user, zabbix_user_directory, zabbix_usergroup - updated to work with Zabbix 6.4. - zabbix_agent role - Add support for SUSE Linux Enterprise Server for SAP Applications ("SLES_SAP"). - zabbix_host - add missing variants for SNMPv3 authprotocol and privprotocol introduced by Zabbix 6 - zabbix_proxy role - Add variable zabbix_proxy_dbpassword_hash_method to control whether you want postgresql user password to be hashed with md5 or want to use db default. When zabbix_proxy_dbpassword_hash_method is set to anything other than md5 then do not hash the password with md5 so you could use postgresql scram-sha-256 hashing method. - zabbix_server role - Add variable zabbix_server_dbpassword_hash_method to control whether you want postgresql user password to be hashed with md5 or want to use db default. When zabbix_server_dbpassword_hash_method is set to anything other than md5 then do not hash the password with md5 so you could use postgresql scram-sha-256 hashing method. - zabbix_usergroup module - userdirectory, hostgroup_rights and templategroup_rights parameters added (Zabbix >= 6.2) - zabbix_web role - possibility to add custom includes in apache vhost config dellemc.powerflex ~~~~~~~~~~~~~~~~~ - Info module is enhanced to support the listing of replication pairs. dellemc.unity ~~~~~~~~~~~~~ - Add synchronous replication support for filesystem. - Support addition of host from the Host List to NFS Export in nfs module. - Support enable/disable advanced dedup in volume module. hetzner.hcloud ~~~~~~~~~~~~~~ - hcloud_image_info - Add cpu architecture field to return value. - hcloud_image_info - Allow filtering images by cpu architecture. - hcloud_server - Select matching image for the cpu architecture of the server type on create & rebuild. - hcloud_server_type_info - Add cpu architecture field to return value. - inventory plugin - Add cpu architecture to server variables. netapp.ontap ~~~~~~~~~~~~ - na_ontap_cifs - new options ``browsable`` and ``show_previous_versions`` added in REST. - na_ontap_cifs - removed default value for ``unix_symlink`` as its not supported with ZAPI. - na_ontap_cifs - updated documentation and examples for REST. - na_ontap_file_security_permissions - updated module examples. - na_ontap_ipspace - improved module fail error message in REST. - na_ontap_rest_info - improved documentation for ``parameters`` option. - na_ontap_security_config - updated documentation for ``supported_cipher_suites``. - na_ontap_user - option ``vserver`` is not required with REST, ignore this option to create cluster scoped user. netbox.netbox ~~~~~~~~~~~~~ - netbox_aggregate - Add tenant as parameter to module - netbox_asn - Add module - netbox_fhrp_group - Add module - netbox_journal_entry - Add module purestorage.flashblade ~~~~~~~~~~~~~~~~~~~~~~ - purefb_info - Added `encryption` and `support_keys` information. - purefb_info - Added bucket quota and safemode information per bucket - purefb_info - Added security update version for Purity//FB 4.0.2, or higher - purefb_info - Updated object store account information - purefb_inventory - Added `part_number` to hardware item information. - purefb_policy - Added support for multiple rules in snapshot policies - purefb_proxy - Added new boolean parameter `secure`. Default of true (for backwards compatability) sets the protocol to be `https://`. False sets `http://` - purefb_s3acc - Added support for default bucket quotas and hard limits - purefb_s3acc - Added support for object account quota and hard limit purestorage.fusion ~~~~~~~~~~~~~~~~~~ - added Python package dependency checks in prerequisites.py - fusion_hap - added missing 'windows' personality type theforeman.foreman ~~~~~~~~~~~~~~~~~~ - content_export_library, content_export_repository, content_export_version - add ``format`` option to control the export format - content_view_filter - add support for creating modulemd filters - content_view_publish role - also accept a list of dicts as the ``content_views`` role for publishing (theforeman/foreman-ansible-modules#1436) - setting - document how to obtain valid setting names (https://bugzilla.redhat.com/show_bug.cgi?id=2174367) Deprecated Features ------------------- cisco.ios ~~~~~~~~~ - ios_bgp_address_family - deprecate redistribute.ospf.match.external with redistribute.ospf.match.externals which enables attributes for OSPF type E1 and E2 routes - ios_bgp_address_family - deprecate redistribute.ospf.match.nssa_external with redistribute.ospf.match.nssa_externals which enables attributes for OSPF type N1 and N2 routes - ios_bgp_address_family - deprecate redistribute.ospf.match.type_1 with redistribute.ospf.match.nssa_externals.type_1 - ios_bgp_address_family - deprecate redistribute.ospf.match.type_2 with redistribute.ospf.match.nssa_externals.type_2
|
I upgraded to the latest version "6.6.0" by running the command ansible-galaxy collection install community.general --upgrade but I am still encountering errors. This time in a different task: Error: |
|
@zoliky The line number at the bottom of that stack trace suggests that the dconf.py that's getting run on the remote host is not the dconf.py in release 6.6.0 of community.general. I can't tell if the upgrade didn't work or if you have multiple copies of dconf.py installed and Ansible is finding the wrong one. If community.general is installed by both the OS and ansible-galaxy, then Ansible may be finding the older one installed by the OS. If you can't untangle that, then you can force it to use the ansible-galaxy version by locating it on disk, creating a directory called "library" in the same directory as the playbook, and putting a symbolic link in that directory pointing at the version of dconf.py you want Ansible to use. |
|
@jikamens Thank you for your response. I appreciate your time. I have recently reinstalled Ansible, and then proceeded to install version 6.6.0 of the I also need to install the But after installing it I get the same error as before: |
|
That's the same stack trace you already posted, so the response I posted above still applies, and I'm not sure why you posted it again after acknowledging that I might have identified the problem correctly. I've already told you how to fix it. Installing psutil may be necessary for other reasons but it has nothing to do with the stack trace you posted. |
|
I managed to fix it by overwritting the system
I'm unsure if there could be any potential long-term negative consequences, but I'm curious if dconf.py is exclusively utilized by Ansible. I attempted to use the 'library' directory and even created a symlink, but that approach appears to have had no effect. I manage my Ansible with Git and utilize ansible-pull. |
👍
I don't think this will be an issue. It's highly unlikely that anything other than Ansible is using this file. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
There are three commits in this PR.
The first is a minor follow-up to #6049 to handle an edge case I didn't anticipate: what happens when the user attempts to set a value which is currently completely unset in the dconf database. We need to handle this explicitly because the GVariant parser throws a TypeError if we attempt to parse None. I don't think this should have a changelog fragment because I don't believe the changes in #6049 have been released yet (please correct me if I'm wrong).
The second is a trivial improvement to an error message which I don't think deserves a changelog entry either. It's just making an error message slightly more verbose to assist in debugging failures. This is mostly of interest to developers of the module, frankly, not to end users.
The third probably does deserve a changelog fragment: I'm adding some intelligence to handle boolean values properly instead of just failing. I'll submit a changelog fragment for that as soon as I have the PR number.
ISSUE TYPE
COMPONENT NAME
dconf