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.

Sign up for GitHub

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

Jump to bottom

Small edgeos_config improvements #199

Merged
merged 4 commits into from
Feb 4, 2021
Merged

Small edgeos_config improvements #199

merged 4 commits into from
Feb 4, 2021

Conversation

jplitza
Copy link
Contributor

@jplitza jplitza commented Feb 1, 2021

SUMMARY
  • The module was littered with magic strings ("set" and "delete") which were too broad a match (because to be commands, they have to be followed by a space).
  • The logical flow to handle when commands started with neither "set" nor "delete" is stated more easily using an "else" block, avoiding duplication of the .startswith() checks
  • Using regular expressions to replace plain strings is overkill
ISSUE TYPE
  • Bugfix Pull Request

(not really, just cleanup)

COMPONENT NAME

edgeos_config

ADDITIONAL INFORMATION

@felixfontein
Copy link
Collaborator

CC @virtualguy

@jplitza can you add a changelog fragment?

@jplitza
edgeos_config: Match space after set/delete commands
7a9b462 
This makes matching the commands a bit more robust and also removes
magic strings from the code by centralizing them in variables.
@jplitza
edgeos_config: Use simple string replacements instead of re
e2ad8ac
@jplitza
edgeos_config: Simplify logical flow
884f2d3 
We do not need to check for each .startswith twice, we can simply use an
else: block
@jplitza
Copy link
Contributor Author

jplitza commented Feb 2, 2021

@jplitza can you add a changelog fragment?

Done, although I only mentioned the only user-visible change (matching the spaces).

@dericcrago dericcrago merged commit 4130c91 into ansible-collections:main Feb 4, 2021
patchback bot pushed a commit that referenced this pull request Feb 4, 2021
@jplitza
Small edgeos_config improvements (#199)
28aeaeb 
* edgeos_config: Match space after set/delete commands

This makes matching the commands a bit more robust and also removes
magic strings from the code by centralizing them in variables.

* edgeos_config: Use simple string replacements instead of re

* edgeos_config: Simplify logical flow

We do not need to check for each .startswith twice, we can simply use an
else: block

* Update changelogs/fragments/199-edgeos-improvements.yaml

Co-authored-by: Felix Fontein <[email protected]>

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 4130c91)
@patchback patchback bot mentioned this pull request Feb 4, 2021
Merged
dericcrago pushed a commit that referenced this pull request Feb 4, 2021
@patchback @jplitza
Small edgeos_config improvements (#199) (#202)
f1566ff 
* edgeos_config: Match space after set/delete commands

This makes matching the commands a bit more robust and also removes
magic strings from the code by centralizing them in variables.

* edgeos_config: Use simple string replacements instead of re

* edgeos_config: Simplify logical flow

We do not need to check for each .startswith twice, we can simply use an
else: block

* Update changelogs/fragments/199-edgeos-improvements.yaml

Co-authored-by: Felix Fontein <[email protected]>

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 4130c91)

Co-authored-by: Jan-Philipp Litza <[email protected]>
@felixfontein
Copy link
Collaborator

@jplitza thanks for improving the module!
@virtualguy @dericcrago thanks for reviewing and merging!

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Apr 2, 2021
ansible: updated to 3.2.0
d789ad6 
v3.2.0

community.crypto
- acme module_utils - the ``acme`` module_utils has been split up into several Python modules (ansible-collections/community.crypto#184).
- acme_* modules - codebase refactor which should not be visible to end-users (ansible-collections/community.crypto#184).
- acme_* modules - support account key passphrases for ``cryptography`` backend (ansible-collections/community.crypto#197, ansible-collections/community.crypto#207).
- acme_certificate_revoke - support revoking by private keys that are passphrase protected for ``cryptography`` backend (ansible-collections/community.crypto#207).
- acme_challenge_cert_helper - add ``private_key_passphrase`` parameter (ansible-collections/community.crypto#207).

community.docker
- docker_swarm_service - change ``publish.published_port`` option from mandatory to optional. Docker will assign random high port if not specified (ansible-collections/community.docker#99).

community.general
- archive - refactored some reused code out into a couple of functions (ansible-collections/community.general#2061).
- csv module utils - new module_utils for shared functions between ``from_csv`` filter and ``read_csv`` module (ansible-collections/community.general#2037).
- ipa_sudorule - add support for setting sudo runasuser (ansible-collections/community.general#2031).
- jenkins_job - add a ``validate_certs`` parameter that allows disabling TLS/SSL certificate validation (ansible-collections/community.general#255).
- kibana_plugin - add parameter for passing ``--allow-root`` flag to kibana and kibana-plugin commands (ansible-collections/community.general#2014).
- proxmox - added ``purge`` module parameter for use when deleting lxc's with HA options (ansible-collections/community.general#2013).
- proxmox inventory plugin - added ``tags_parsed`` fact containing tags parsed as a list (ansible-collections/community.general#1949).
- proxmox_kvm - added new module parameter ``tags`` for use with PVE 6+ (ansible-collections/community.general#2000).
- rax - elements of list parameters are now validated (ansible-collections/community.general#2006).
- rax_cdb_user - elements of list parameters are now validated (ansible-collections/community.general#2006).
- rax_scaling_group - elements of list parameters are now validated (ansible-collections/community.general#2006).
- read_csv - refactored read_csv module to use shared csv functions from csv module_utils (ansible-collections/community.general#2037).
- redfish_* modules, redfish_utils module utils - add support for Redfish session create, delete, and authenticate (ansible-collections/community.general#1975).
- snmp_facts - added parameters ``timeout`` and ``retries`` to module (ansible-collections/community.general#980).
- vdo - add ``force`` option (ansible-collections/community.general#2101).

community.network
- edgeos_config - match the space after ``set`` and ``delete`` commands (ansible-collections/community.network#199).
- nclu - execute ``net commit description <description>`` only if changed ``net pending``'s diff field (ansible-collections/community.network#219).

community.postgresql
- postgresql_info - add the ``patch``, ``full``, and ``raw`` values of the ``version`` return value (ansible-collections/community.postgresql#68).
- postgresql_ping - add the ``patch``, ``full``, and ``raw`` values of the ``server_version`` return value (ansible-collections/community.postgresql#70).

community.zabbix
- zabbix_agent - added support for installations on arm64 systems (ansible-collections/community.zabbix#320).
- zabbix_proxy - now supports configuring StatsAllowedIP (ansible-collections/community.zabbix#337).
- zabbix_server - added support for installtions on arm64 systems (ansible-collections/community.zabbix#320).
- zabbix_web - added support for installtions on arm64 systems (ansible-collections/community.zabbix#320).

dellemc.openmanage
- ome_template - Allows to deploy a template on device groups.

hetzner.hcloud
- Add firewalls to hcloud_server module

ovirt.ovirt
- cluster_upgrade - Add correlation-id header (oVirt/ovirt-ansible-collection#222).
- engine_setup - Add skip renew pki confirm (oVirt/ovirt-ansible-collection#228).
- examples - Add recipe for removing DM device (oVirt/ovirt-ansible-collection#233).
- hosted_engine_setup - Filter devices with unsupported bond mode (oVirt/ovirt-ansible-collection#226).
- infra - Add reboot host parameters (oVirt/ovirt-ansible-collection#231).
- ovirt_disk - Add SATA support (oVirt/ovirt-ansible-collection#225).
- ovirt_user - Add ssh_public_key (oVirt/ovirt-ansible-collection#232)

purestorage.flasharray
- purefa_maintenance - New module to set maintenance windows
- purefa_pg - Add support to rename protection groups
- purefa_syslog - Add support for naming SYSLOG servers for Purity//FA 6.1 or higher

purestorage.flashblade
- purefb_certs - Add update functionality for array cert
- purefb_fs - Add multiprotocol ACL support
- purefb_info - Add information regarding filesystem multiprotocol (where available)
- purefb_info - Add new parameter to provide details on admin users
- purefb_info - Add replication performace statistics
- purefb_s3user - Add ability to remove an S3 users existing access key
@Andersson007
Copy link
Contributor

@jplitza hi, would you like to have your GH login added to .github/BOTMETA.yml as a maintainer of the module?
You'll be notified about Issues/PRs related to the module and your shipit will be counted by bot for automerge (needs two for bugfixes and minor changes).
what do you think?

@jplitza jplitza deleted the edgeos_improvements branch April 21, 2021 07:43
@jplitza
Copy link
Contributor Author

jplitza commented Apr 21, 2021

@Andersson007 I certainly know the EdgeOS devices well enough, and probably also the module's code. However, I feel unsure about the whole development ecosystem (shipit? Changelog Fragments?), so I'd be glad if you have any pointers in that regard. But in general, yeah, sure, you can put me up for everything that has "edgeos" (or "edgeswitch", for that matter) in its name!

@Andersson007
Copy link
Contributor

@jplitza thanks!

Answering your question:

  1. We have the bot I mentioned in this collection (as well as in community.general and others) which is tracking the issues/PRs including for keywords such as shipit, the full list is here.

  2. If your GH login is listed in .github/BOTMETA.yml as a maintainer of some stuff and you'll put shipit in a PR that touches that stuff, the bot will count your shipit. If another maintainer also puts shipit (so we need two), the PR will be merged by the bot automatically - so being a maintainer in BOTMETA.yml is a kind of indirect write access.

  3. BOTMETA.yml maintainers who are active and provide regular significant contributions (reviews and code contributions) in collection scope can be granted the write access to the repository: they can merge others PRs, release the collection, etc.

  4. We have the review checklist and contributing guide. It's highly recommended to read these documents. Also a maintainer guidelines is being in development.

Thank you!

@Andersson007 Andersson007 mentioned this pull request Apr 22, 2021
Merged
@Andersson007
Copy link
Contributor

@jplitza could you please approve the PR #256

@Andersson007 Andersson007 mentioned this pull request Apr 22, 2021
Merged
This was referenced Apr 22, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein approved these changes

@virtualguy virtualguy virtualguy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jplitza @felixfontein @Andersson007 @virtualguy @dericcrago