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

SNMP model #280

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

SNMP model #280

wants to merge 36 commits into from

Conversation

aida-shumburo
Copy link

No description provided.

@aida-shumburo aida-shumburo marked this pull request as ready for review October 22, 2024 16:42
Copy link
Contributor

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am posting several comments requesting some minor changes, but this model is structured correctly and is a suitable base for starting the "module" implementation.

models/enterprise_sonic/snmp/sonic_snmp.yml Show resolved Hide resolved
models/enterprise_sonic/snmp/sonic_snmp.yml Show resolved Hide resolved
models/enterprise_sonic/snmp/sonic_snmp.yml Show resolved Hide resolved
models/enterprise_sonic/snmp/sonic_snmp.yml Show resolved Hide resolved
models/enterprise_sonic/snmp/sonic_snmp.yml Show resolved Hide resolved
models/enterprise_sonic/snmp/sonic_snmp.yml Show resolved Hide resolved
Copy link
Contributor

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the PR content with the most recent changes. Some of my previously posted "Issues" have been resolved with the changes. But there are some remaining issues. I have posted additional information to explain these issues.

models/enterprise_sonic/snmp/sonic_snmp.yml Show resolved Hide resolved
models/enterprise_sonic/snmp/sonic_snmp.yml Show resolved Hide resolved
models/enterprise_sonic/snmp/sonic_snmp.yml Outdated Show resolved Hide resolved
models/enterprise_sonic/snmp/sonic_snmp.yml Show resolved Hide resolved
Copy link
Contributor

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am posting some requests for changes in the "Examples" files.

After these changes are made, we can proceed with a request to get this model merged.

models/enterprise_sonic/snmp/deleted_example_01.txt Outdated Show resolved Hide resolved
models/enterprise_sonic/snmp/deleted_example_01.txt Outdated Show resolved Hide resolved
models/enterprise_sonic/snmp/merged_example_01.txt Outdated Show resolved Hide resolved
models/enterprise_sonic/snmp/merged_example_01.txt Outdated Show resolved Hide resolved
models/enterprise_sonic/snmp/overridden_example_01.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@ArunSaravananBalachandran ArunSaravananBalachandran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the example files, please ensure that the sample playbook is not commented out.

models/enterprise_sonic/snmp/sonic_snmp.yml Outdated Show resolved Hide resolved
models/enterprise_sonic/snmp/sonic_snmp.yml Show resolved Hide resolved
models/enterprise_sonic/snmp/sonic_snmp.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am posting some additional comments requesting changes.

I have checked the changes made for my previous set of comments on the "Examples" section. The requested changes go most of the way toward addressing the comments, but there are still a few related corrections to make. Also, in checking the changes, I realized that an additional change is needed in the model (.yaml) file to correctly represent the mutually exclusive nature of user "priv" key types. (This is very similar to the problem already addressed for user "auth" key types.)

Copy link
Contributor

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest revisions correctly address the previous set of change requests. I am requesting only two very minor additional adjustments to the descriptive comments.

description:
- Type of encryption
- Choices are 'aes' or 'des'
type: str
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with the "auth_type" option, please add a "choices" attribute here instead of only stating it in the description. This will eliminate the need to check this in the executable code.

(The description part for the choices is optional if an explicit "choices" attribute is used.)


- name: Delete all SNMP communities
- name: Delete all SNMP server configuration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify that the maximum scope of deletion is a single top level sub-option.

Suggested change
- name: Delete all SNMP server configuration
- name: Delete all SNMP server configuration for the "user" sub-option.

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.

4 participants