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 nctalk adapter to latest #2150

Closed
wants to merge 2 commits into from
Closed

Add nctalk adapter to latest #2150

wants to merge 2 commits into from

Conversation

jjqoie
Copy link

@jjqoie jjqoie commented Feb 23, 2023

No description provided.

@jjqoie jjqoie changed the title Added nctalk nextclould talk adapter Added nctalk nextcloud talk adapter Feb 23, 2023
@Apollon77 Apollon77 added the REVIEW needed (apollon77) A developer from the ioBroker Team will review the adapter, will provide comments or require changes label Feb 23, 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 Feb 23, 2023
@ioBroker ioBroker deleted a comment from Apollon77 Feb 23, 2023
@jjqoie
Copy link
Author

jjqoie commented Feb 23, 2023

Hi
[E518] "@alcalzone/release-script" is used, but ".releaseconfig.json" not found
seems this came with a newer version of "@alcalzone/release-script" - what should I do now ? update to latest "@alcalzone/release-script"package ?
Anything special to take care of when updating or?

UPDATE:
This is the commit for updating the latest "@alcalzone/release-script" development package and adding ".releaseconfig.json" - as I'm unsure if this is correct what I did, it would be nice if someone can quickly check and comment -thx
jjqoie/ioBroker.nctalk@7c2d57e

thanks
best regards
Jochen

@ioBroker ioBroker deleted a comment from jjqoie Feb 23, 2023
@ioBroker ioBroker deleted a comment from jjqoie Feb 23, 2023
@ioBroker ioBroker deleted a comment from jjqoie Feb 23, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented Feb 24, 2023

UPDATE: This is the commit for updating the latest "@alcalzone/release-script" development package and adding ".releaseconfig.json" - as I'm unsure if this is correct what I did, it would be nice if someone can quickly check and comment -thx jjqoie/ioBroker.nctalk@7c2d57e

thanks best regards Jochen

Looks good as far as I can determine (but note, thats not an official statement from kernel experts!)

Please use the manual review possibility to check (and correct if required) the translated text of releasenotes before letting the script continue and commit to avoid funny or misunderstanding german translations.

P.S: Are you sure that adding the adapter to latest AND stable at the same time is OK? That's nothing I can or ware willing to decide but its a little bit unusual.

@jjqoie
Copy link
Author

jjqoie commented Feb 24, 2023

HI Martin,
thanks for checking :-)

On your last point - yes I was not sure about this point as well and about the prerequisite for "stable"...
For me the adapter runs "stable" :-D and it was also tested/used by several others (~20 if this number can be trusted) for approx 1 year and at the beginning quite some debugging was required but then there were no further problems reported so far... here the forum entry https://forum.iobroker.net/topic/49298/neuer-adapter-nextcloud-talk-messenger

best regards
Jochen

@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
@ioBroker ioBroker deleted a comment from mcm1957 May 20, 2023
@mcm1957
Copy link
Collaborator

mcm1957 commented May 22, 2023

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

In addition PLEASE REMOVE sources-dist-stable.json from this PR. Adapters MUST be some time available at lastest before they are allowed to be added to stable. So adding a new adapter to latest AND stable at the same time is inapproriate.

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
@ioBroker ioBroker deleted a comment from jjqoie Jun 2, 2023
@jjqoie
Copy link
Author

jjqoie commented Jul 25, 2023

Any news here?

@mcm1957
Copy link
Collaborator

mcm1957 commented Jul 25, 2023

Any news here?

Sorry to say still waiting for official review by @Apollon77 .

@mcm1957 mcm1957 added RE-REVIEW pending (by 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 Jul 25, 2023
@Apollon77
Copy link
Collaborator

Ok, short review:

  • Please remove the developer things (adapter development) from reamde ... This will confuse the adapter users
  • I think the new to 0.1.0 are broken in io-package... license shopuld not be in there. maybe remove it
  • Please make sure to check ack flag on Stage change cases. ack= false are commands to be executed. ack=true are just value changes that should not trigger any action!
  • in fact this nctalk library has one big downside ... it never stops. The timeouts used in there are not tracked and can not be stopped, soideally that should be enhanced because else this will create big issues in compact mode

@Apollon77 Apollon77 added must be fixed The Adapter request got review/automatic feedback that is required to be fixed before another review and removed REVIEW needed (apollon77) 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 RE-REVIEW pending (by mcm1957) Changes requested by review have been applied, re-review could be done. labels Jul 26, 2023
@jjqoie
Copy link
Author

jjqoie commented Jul 26, 2023

Ok 1. and 2nd is done
on the 3rd - is this the sequence how it should be?
if ack == false
then for example sent message to nextcloud talk
setstate ack to true indicating we have handled the event


onStateChange(id, state) {

       const as = id.split(".");
       const statename = as[as.length - 1];
       let obj = undefined;
       let fullpath = this.adapter.config.upload_path;


       switch (statename) {
           case "SendMessage":
               if (state.ack == false) {
                   // ack==false are commands to be executed
                   this.SendToGroup(state.val);
                   this.adapter.setState(id, state.val, true);
               }
               break;

jjqoie added a commit to jjqoie/ioBroker.nctalk that referenced this pull request Jul 26, 2023
jjqoie added a commit to jjqoie/ioBroker.nctalk that referenced this pull request Jul 26, 2023
@jjqoie
Copy link
Author

jjqoie commented Feb 22, 2024

hi
still working on the "nextcloud library and timeouts" item, which takes a bit longer as I also want to take care of all active http requests also in the other states used during startup.
Could it be that using the "dev-server watch", that the adapter can't be stopped?
BR
Jochen

@github-actions github-actions bot added the *📬 a new comment has been added label Feb 22, 2024
@mcm1957
Copy link
Collaborator

mcm1957 commented Feb 22, 2024

Thanks for feedback. As long you are working on the adapter everything is fine.

I do not really undersatnd you question. In general the dev-server should show the same behavior as the normal js-controller. Maybe you could try dev-server run which does not start the adapter and start the adapter afterwards using you ide (i.e. vs-code). But discussing dev-server is a little bit OT here. Please join our telegram groups (or discord) You find links at www.iobroker.dev. There should be devs to discuss the specific problem with you and help solvong it.

reminder 01.04.2024

@mcm1957 mcm1957 removed stale PR seems has no activity, will be closed after some time *📬 a new comment has been added labels Feb 22, 2024
@mcm1957
Copy link
Collaborator

mcm1957 commented Apr 3, 2024

Any news here?
Do you know any timeframe wh to expect a new revier after your changes?

reminder 1.5.2024

@mcm1957 mcm1957 added the stale PR seems has no activity, will be closed after some time label Apr 3, 2024
@github-actions github-actions bot added 1.5.2024 and removed 1.4.2024 labels Apr 3, 2024
@mcm1957
Copy link
Collaborator

mcm1957 commented May 5, 2024

Any news here?
Do you know any timeframe wh to expect a new review after your changes?

Please let us know whether you are still working at this adapter. Let us know if you need some help or simply more time.

If there is no reaction until 31.5.2024 we will consider closing this PR. (Of course you are welcome to open a new PR at any time.)

reminder 15.5.2024

@mcm1957
Copy link
Collaborator

mcm1957 commented May 17, 2024

Any news here?
Do you know any timeframe wh to expect a new review after your changes?

Please let us know whether you are still working at this adapter. Let us know if you need some help or simply more time.

If there is no reaction until 31.5.2024 we will consider closing this PR. (Of course you are welcome to open a new PR at any time.)

reminder 1.6.2024

@mcm1957 mcm1957 added stale - marked for closing There was no feedback vom PR owner, PR will be closed. 15.5.2024 and removed stale PR seems has no activity, will be closed after some time 1.6.2024 labels May 17, 2024
@mcm1957
Copy link
Collaborator

mcm1957 commented Jun 1, 2024

As there was no reaction this PR has been closed.

Please feel free to open a new PR as soon as you think the adapter is ready for a new review

@mcm1957 mcm1957 closed this Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 stale - marked for closing There was no feedback vom PR owner, PR will be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants