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 gotify to latest #2462

Merged
merged 1 commit into from
Aug 21, 2023
Merged

Conversation

ThomasPohl
Copy link
Contributor

Hi, I have developed an iobroker adapter to send messages using gotify.

There is a thread in the forum:
https://forum.iobroker.net/topic/43305/interesse-an-gotify-adapter/39

I hope this is the correct way to add a new adpater. If there is anything that needs to be done to continue, please let me know.

@github-actions github-actions bot added the auto-checked This PR was automatically checked for obvious criterias label Jul 26, 2023
@github-actions
Copy link

Automated adapter checker

ioBroker.gotify

Downloads Number of Installations (latest)
NPM

👍 No errors found

  • 👀 [W156] Adapter should support admin 5 UI (jsonConfig) if you do not use a React based UI
  • 👀 [W400] Cannot find "gotify" in latest repository

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

@mcm1957
Copy link
Collaborator

mcm1957 commented Jul 26, 2023

Yes this is the right way.
Please note that a Review by core team will be performed before merging yozr PR. Due to current load this might last some time.

@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 3, 2023

First of all - THANK YOU for the time and effort you spend to maintain this adapter.

I would like to give some (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 suggestions or before you spend major effort.

  • Readme.md does not contain changelog for latest release

    The changelog at README.md of this adapter does not contain an entry for the current release available at npm. Please update README.md. (A new release is not required, an update at github is sufficient.)

  • consider using axios timeouts

    Consider setting a reasonable timeout for axios post request to avoid waiting forever.

    According to axios documentation the default value for timeout is 0 which means no timeout
    https://github.com/axios/axios/blob/main/README.md#request-config

    // `timeout` specifies the number of milliseconds before the request times out.
    // If the request takes longer than `timeout`, the request will be aborted.
    timeout: 1000, // default is `0` (no timeout)
    
  • adapt testing to supported node releases

    Consider adding node 20 tests unless you already know incompatibilities.

    The recommended testmatrix is [16.x, 18.x, 20.x],

Thanks for reading and evalutaing 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 added the must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review label Aug 3, 2023
@ThomasPohl
Copy link
Contributor Author

Thank you for the review.

  • I updated the tests to run in node 16, 18, and 20.
  • I added the timeout to the axios call. (Sounds useful )
  • I commented all the changes (README and io-package.json) and released them as v0.2.0

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

mcm1957 commented Aug 6, 2023

Small hint for future releases:

Normally the latest (newest) release info is on top at README.md. I suggest you reorder the entries. In addition I recommedn @alcalzone/release-script which could support creating a release and update all required files.

Looks like you did not use @alcalzone/release-script. It seems to be installed already, so I recommend to take a look at it - should make releasing easier in most environments.

@mcm1957 mcm1957 added lgtm Looks Good To Me and removed (RE-)REVIEW pending (mcm1957) Changes requested by review have been applied, re-review could be done. labels Aug 6, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 6, 2023

lgtm now

@mcm1957 mcm1957 added the Adapter-Review needed A developer from the ioBroker Team will review the adapter, will provide comments or require changes label Aug 7, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Aug 21, 2023

@ThomasPohl
This adapter has been released to latest repository and should be visible within 24h maximum.

Please create a thread at https://forum.iobroker.net/category/91/tester titled like "Test Adapter " to collect some user feedback and provide a link to this topic when requesting addition to stable repository later.

Note: If an other testing topic already exists, ist OK to continue using this topic too.

@mcm1957 mcm1957 merged commit 76baa92 into ioBroker:master Aug 21, 2023
12 checks passed
@ThomasPohl ThomasPohl deleted the feature/gotify-repository branch August 22, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Adapter-Review needed A developer from the ioBroker Team will review the adapter, will provide comments or require changes auto-checked This PR was automatically checked for obvious criterias lgtm Looks Good To Me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants