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

Add my-opcua to latest #2693

Closed
wants to merge 1 commit into from

Conversation

gokturk413
Copy link

Please add my adapter ioBroker.my-opcua to latest.

This pull request was created by https://www.iobroker.dev c0726ff.

@github-actions github-actions bot added the auto-checked This PR was automatically checked for obvious criterias label Sep 29, 2023
@mcm1957 mcm1957 added the RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. label Sep 29, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Sep 29, 2023

@gokturk413

Thanks for writing an adapter for ioBroker.

Before I will do a detailled review, please check th requirements for adding an adapter to latest repository.
https://github.com/ioBroker/ioBroker.repositories#requirements-for-adapter-to-get-added-to-the-latest-repository
https://github.com/ioBroker/ioBroker.repositories#development-and-coding-best-practices

Especially

  • fix issues reported by adapter checker
  • fix standard github testing (testing MUST be present and pass tests for node (16) / 18 / 20
  • readme must cobtain a suffiient desctiption of the functionality the adapetr provides incuding a link to the manufakturer of device or service. "install .net framework 4.6 , Node.js v18.14.0" is too few information about installation and usage
  • non relevant directoies (i.e. log) should be removed from github repository
  • all dummy and example code must be removed, logs must be meaningful
  • Code must have a working functionality. I cannot see i.e. setState do something useful.

Please add a comment when you have reviewed and fixed the suggestions at least commented the suggestions and you think the adapter is ready for a review!

@mcm1957 mcm1957 added must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review and removed RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. labels Sep 29, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Sep 30, 2023

@gokturk413
There's an existing adapter ioBroker.opcua.

Please check wether this adapter will satisfy your needs. If you want to add some features, please contact @GermanBluefox (i.e. using a issue at this repo) and / or submit an PR.

I will close this PR as there are several issues to be fixed and the the need for a new adapter is notz clear.
If you really see a benefit creating a new adapter, please discuss with @GermanBluefox, create a new PR and clearly state why we should add a new adapter.

@Apollon77 FYI

@mcm1957 mcm1957 closed this Sep 30, 2023
@mcm1957 mcm1957 added cancellation of PR suggested The adapter needs significant rework before a review seems to be appropiate wontfix and removed must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review auto-checked This PR was automatically checked for obvious criterias labels Sep 30, 2023
@gokturk413
Copy link
Author

gokturk413 commented Oct 1, 2023 via email

@mcm1957
Copy link
Collaborator

mcm1957 commented Oct 1, 2023

Ok,I'll check the commercial aspect and come back. I did not notice this.

Not working well should normally result in one or more issues or PR. In respect to commercial license this will be considered different.

I'll reipen PR for now. Please let us / me know at which timeframe you plan to solve the first code issues reported (see above). Please note that a detailled review will follow after vasic errors are fixed.

@Apollon77 FYi

@mcm1957 mcm1957 reopened this Oct 1, 2023
@github-actions github-actions bot added the auto-checked This PR was automatically checked for obvious criterias label Oct 1, 2023
@github-actions
Copy link

github-actions bot commented Oct 1, 2023

Automated adapter checker

ioBroker.my-opcua

Downloads Number of Installations (latest)
NPM

👍 No errors found

  • 👀 [W171] "common.title" is deprecated in io-package.json
  • 👀 [W105] "common.titleLang" should be translated into all supported languages (en, de, ru, pt, nl, fr, it, es, pl, uk, zh-cn)
  • 👀 [W109] "common.desc" should be translated into all supported languages (en, de, ru, pt, nl, fr, it, es, pl, uk, zh-cn)
  • 👀 [W156] Adapter should support admin 5 UI (jsonConfig) if you do not use a React based UI
  • 👀 [W145] Each "common.news" should be translated into all supported languages (en, de, ru, pt, nl, fr, it, es, pl, uk, zh-cn)
  • 👀 [W400] Cannot find "my-opcua" in latest repository

Add comment "RE-CHECK!" to start check anew

@mcm1957 mcm1957 added must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. ON HOLD PR is set ON HOLD due to pending question or major issues. and removed cancellation of PR suggested The adapter needs significant rework before a review seems to be appropiate wontfix labels Oct 1, 2023
@gokturk413
Copy link
Author

gokturk413 commented Oct 2, 2023 via email

@mcm1957
Copy link
Collaborator

mcm1957 commented Oct 2, 2023

As a PR is not the right place to discuss basic development issues, I woul suggest to join iobroker development starters group to get help for development and basics.

https://t.me/+gsX-e8k4mLtmZjZk

Please mention your goals (devlop adapter xxy) after subscribing as without any iobroker related communication you might be kicked out very fast (spam bot measurement).

I've provided a PR to update test-and-release script ot current level and versions. Normall you should not need to modify anything inside the workflow.

Memory problems at eslint are unusual. Maybe they are cuased by outdated node 14 (fixed by PR mentioned) or some serious code problem confusing eslint. Please ask at Forum if still persist with node 16/18/20.

@mcm1957
Copy link
Collaborator

mcm1957 commented Oct 4, 2023

Please add a comment when you have reviewed and fixed the suggestionsor at least commented the suggestions and you think the adapter is ready for a re-review!

@mcm1957 mcm1957 removed RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. ON HOLD PR is set ON HOLD due to pending question or major issues. labels Oct 4, 2023
@gokturk413
Copy link
Author

gokturk413 commented Oct 9, 2023 via email

@mcm1957
Copy link
Collaborator

mcm1957 commented Oct 9, 2023

As a PR is not the right place to discuss basic development issues, I would suggest to join iobroker development starters group at telegram to get help for development and basics.

https://t.me/+gsX-e8k4mLtmZjZk

Please mention your goals (devlop adapter xxy) after subscribing as without any iobroker related communication you might be kicked out very fast (spam bot measurement).

@mcm1957
Copy link
Collaborator

mcm1957 commented Oct 9, 2023

I will close here as there is still some work to be done before a first release is available.

Please open a new PR after standard github action pass the test and the adapter is working at your installation. I also suggest that you open a Topic at forum.iobroker.net within tester area to request additional test by some users (although this is not required for a latest PR.

I repeat my very first review results (Some points might be solver - I did no new review lately)

  • fix issues reported by adapter checker
  • fix standard github testing (testing MUST be present and pass tests for node (16) / 18 / 20
  • readme must cobtain a suffiient desctiption of the functionality the adapetr provides incuding a link to the manufakturer of device or service. "install .net framework 4.6 , Node.js v18.14.0" is too few information about installation and usage. Add an example how to use the adapter to readme.
  • non relevant directories (i.e. log) should be removed from github repository
  • all dummy and example code must be removed, logs must be meaningful
  • Code must have a working functionality. I cannot see i.e. setState do something useful.

For support please join our telegram channel - see link above.

McM1957

@mcm1957 mcm1957 closed this Oct 9, 2023
@mcm1957 mcm1957 removed the auto-checked This PR was automatically checked for obvious criterias label Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants