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 fully-mqtt to latest #2184

Closed
wants to merge 1 commit into from
Closed

Add fully-mqtt to latest #2184

wants to merge 1 commit into from

Conversation

Acgua
Copy link

@Acgua Acgua commented Mar 19, 2023

Please add https://github.com/Acgua/ioBroker.fully-mqtt to latest.

Following error will appear when executing adapter check:

[E201] Bluefox was not found in the collaborators on NPM!. Please execute in adapter directory: "npm owner add bluefox iobroker.fully-mqtt"

I have executed the command, but invite has not yet been confirmed by bluefox.

@Apollon77 Apollon77 added the REVIEW needed (apollon77) A developer from the ioBroker Team will review the adapter, will provide comments or require changes label Mar 19, 2023
@GermanBluefox GermanBluefox added auto-checked This PR was automatically checked for obvious criterias must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review labels Mar 19, 2023
@ioBroker ioBroker deleted a comment from Apollon77 Mar 19, 2023
@ioBroker ioBroker deleted a comment from mcm1957 Mar 28, 2023
@ioBroker ioBroker deleted a comment from Acgua Apr 4, 2023
@Acgua
Copy link
Author

Acgua commented Apr 4, 2023

"must be fixed" label can be removed now. I have deleted the bluefox invite, and re-invited him, and in the meantime he is listed as maintainer on npm.

However, please note that I am currently working on reported issue #25. I will need to change src/lib/mqtt-server.ts, several improvements are needed.

So, it may make sense that you wait with the adapter review until my changes and fixes are in place.
I will post a new comment once completed.

Thanks.

image

@mcm1957 mcm1957 removed the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label May 18, 2023
@GermanBluefox
Copy link
Contributor

Automated adapter checker

ioBroker.fully-mqtt

Downloads Number of Installations (latest)
NPM

👍 No errors found

  • 👀 [W400] Cannot find "fully-mqtt" in latest repository

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

@ioBroker ioBroker deleted a comment from mcm1957 May 20, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented May 22, 2023

Please remove node 14 from test-and-release.yml and (optionally) add node 20 to testing matrix.
Node 14 is no longer supported and tests will fail since 22.5.2023.

Thanks
McM1957

@mcm1957 mcm1957 added the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label May 22, 2023
@mcm1957 mcm1957 added RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. and removed REVIEW needed (apollon77) A developer from the ioBroker Team will review the adapter, will provide comments or require changes labels Aug 5, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 8, 2023

I would like to give some additional (preliminary) feedback based on my personal oppinion. This is NOT an offical review and @Apollon77 might have several additional suggestions or even a different oppinion to one or the other statement. Please feel free to contact him (or wait for a response from him) if you cannot follow my suggestsions or before you spend major effort.

  • adapt testing to supported node releases

    As node 14 is end of life and will no longer be supported by js-controller 5.x, consider removing node 14.x tests from matrix id still present.
    Tests for node 16 is mandatory unless you require node 18 and higher.
    Tests for node 18 is mandatory as many users already use node 18 and node 16 will be eol and deprected Q3/2023.
    Consider adding node 20 tests unless you already know incompatibilities.

    So the recommended testmatrix is [16.x, 18.x, 20.x],

  • reenable dependabot

    Due to failing node 14 tests dependabot PRs have been queued up. Please reenable dependabot.
    In addition I would suggest to update pending dependencies and create a new release with actual dependencies,
    (Note: This is a recommandation only and will not block adding to latest while updating test matrix is mandatory).

Otherwise the adapter looks good to me.

Thanks for reading and evaluating this suggestions.
McM1957

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 the RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. label Aug 8, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Sep 7, 2023

How shall we continue?
Please let us know if you plan zu fix the noted issues in the near future - or as an alterntive - like to discuss some / all of those points.

@mcm1957 mcm1957 added the question Something needs to be done or answered by the adapter developer label Sep 7, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Oct 8, 2023

@mcm1957
Copy link
Collaborator

mcm1957 commented Oct 8, 2023

@Acgua

How shall we continue?
Please let us know if you plan zu fix the noted issues in the near future - or as an alterntive - like to discuss some / all of those points.

If we do not receive any responce within 30 days we will close this PR

@mcm1957 mcm1957 added the stale PR seems has no activity, will be closed after some time label Oct 8, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Nov 12, 2023

I will close this PR now.
No reaction received.

mqtt functionality is now integrated into adapter fully-browser

Please feel free to open a new PR if you continue developmengt of your adapter,

@mcm1957 mcm1957 added wontfix and removed question Something needs to be done or answered by the adapter developer 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 Nov 12, 2023
@mcm1957 mcm1957 closed this Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR seems has no activity, will be closed after some time wontfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants