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

uplink_olt_model_add #3231

Merged
merged 10 commits into from
Oct 2, 2024
Merged

uplink_olt_model_add #3231

merged 10 commits into from
Oct 2, 2024

Conversation

AAm-kun
Copy link
Contributor

@AAm-kun AAm-kun commented Jul 18, 2024

Description

Added UPLINK olt support. V-SOL olt also might work as uplink is rebrand of v-sol in some way. not tested on V-SOL though.

Some part of the code was taken from https://gist.github.com/mdpuma/0df4bc4fe6dd8d0aad6d0445902a9f63 and some was generated by Mistral AI.

P:S: Not a ruby dev, just a network engineer who was able to make it work and wanted to share with the community.

@robertcheramy
Copy link
Collaborator

Please document your change in CHANGELOG.md and add the model in docs/Supported-OS-Types.md

The code must pass rubocop and rake test.
Have a look at: https://github.com/ytti/oxidized/blob/master/CONTRIBUTING.md#code-like-a-ruby-professional

@AAm-kun
Copy link
Contributor Author

AAm-kun commented Aug 15, 2024

Please document your change in CHANGELOG.md and add the model in docs/Supported-OS-Types.md

The code must pass rubocop and rake test. Have a look at: https://github.com/ytti/oxidized/blob/master/CONTRIBUTING.md#code-like-a-ruby-professional

Finally made time to do the test and updated the files today. Let me know if everything is fine or not.
Thank you.

@systeembeheerder
Copy link
Contributor

@AAm-kun you should ask @robertcheramy to review, not me.

I find i hard to find any information on the vendor 'uplink' and 'EP4440' seems a specific device, not an network OS. Is OLT the OS then? But yet you add EP4440 to the OS list. Just my two cents.

@AAm-kun
Copy link
Contributor Author

AAm-kun commented Aug 15, 2024

@systeembeheerder Well Uplink is a rebrand of VSOL if I am not wrong. The OS is same, at least what I saw the webui in youtube videos.
Was not sure what to put in the OS, as there are nothing about this device on google. But saw in the supported list there were some os same name as devices that was the reasoning for putting EP4440. EP4440 is not a specific device either, it is a prefix of multiple devices which shares the same OS. This is basically what my device shows.
image

@robertcheramy please review when you have time.

@robertcheramy
Copy link
Collaborator

The PR looks good, I will be happy to merge it into master after following changes:

  • Please provide a unit test for the model. You will need to rebase / merge from master in order to use model_helper.rb in the unit test.
  • Please document your changes in CHANGELOG.md

@robertcheramy
Copy link
Collaborator

Update - consider the unit test point as optional - making the yaml file is to complicated yet, I must improve the documentation before asking submitter do to so.
Please just document your changes in CHANGELOG.md

@AAm-kun
Copy link
Contributor Author

AAm-kun commented Sep 28, 2024

Update - consider the unit test point as optional - making the yaml file is to complicated yet, I must improve the documentation before asking submitter do to so. Please just document your changes in CHANGELOG.md

change-log has been added. Let me know if anything else is missing.

- Author of the model in CHANGELOG.md
- Supported OS Types in alphabetical order
@robertcheramy
Copy link
Collaborator

Sorry for the long delay - I was busy with other topics.

@robertcheramy robertcheramy merged commit b253cd6 into ytti:master Oct 2, 2024
5 checks passed
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