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.

Sign up for GitHub

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

Jump to bottom

Adds some extra commands to the cln-plugin #83

Merged
merged 2 commits into from
Aug 22, 2022
Merged

Adds some extra commands to the cln-plugin #83

merged 2 commits into from
Aug 22, 2022

Conversation

sr-gi
Copy link
Member

@sr-gi sr-gi commented Jul 28, 2022

Changes:

  • abandontower <tower_id> will remove all data associated with a given tower. If the tower is being retried, the retry strategy will be aborted on the next iteration.
  • getappointmentreceipt <tower_id> <locator> will pull a given appointment receipt from the local database.
  • getregistrationreceipt <tower_id> will pull the latest registration receipt from the local database.

Followups:

  • Add some optional parameters to getregistrationreceipt so a period can be specified. If present, the receipt matching that period will be pulled (if any).

@sr-gi sr-gi added feature request New feature or request Seeking Code Review review me pls cln-plugin Stuff related to watchtower-plugin labels Jul 28, 2022
@sr-gi sr-gi added the Failing on CI :( label Aug 8, 2022
@sr-gi sr-gi force-pushed the cln-plugin-additional-commands branch 3 times, most recently from 176cb95 to a8e2857 Compare August 11, 2022 14:19
@sr-gi sr-gi mentioned this pull request Aug 11, 2022
Merged
@sr-gi
Copy link
Member Author

sr-gi commented Aug 11, 2022

The CI issues showing up here should have been fixed in #84. This actually revealed a race condition that was affecting the plugin.

@sr-gi sr-gi added this to the v.0.1.2 milestone Aug 15, 2022
mariocynicys
mariocynicys requested changes Aug 15, 2022
View reviewed changes
Copy link
Collaborator

@mariocynicys mariocynicys left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tiny nit and missing docs about the new commands.
I also feel that the new state abandoned is too short lived to be mentioned in the docs, but on the other hand, will be so confusing if some user seen it while getingtowerinfo, because It won't be clear whether the plugin deletes that data or keeps it and only flags the tower.

I think we can remove the tower right when the user asks (in abandontower callback) and manage it in the retrier not to retry towers which are not found in wt_client.towers.
What do you think about that?

watchtower-plugin/src/retrier.rs Outdated
.is_abandoned()
{
log::info!("Abandoning {}", tower_id);
println!("Abandoning {}", tower_id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the print?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

@sr-gi
Copy link
Member Author

sr-gi commented Aug 16, 2022

@meryacine I guess we could get rid of TowerStatus::Abandoned and just check whether wt_client.towers.get(&tower_id) exists.

@sr-gi sr-gi force-pushed the cln-plugin-additional-commands branch from a8e2857 to 4919302 Compare August 16, 2022 09:51
@sr-gi
Copy link
Member Author

sr-gi commented Aug 16, 2022

@meryacine fbce39f should have fixed both issues you mentioned.

You mentioned another issue with the docs, but I couldn't see the comment. lmk if there is anything missing.

@sr-gi sr-gi force-pushed the cln-plugin-additional-commands branch from 4919302 to be0c671 Compare August 16, 2022 09:56
@mariocynicys
Copy link
Collaborator

You mentioned another issue with the docs, but I couldn't see the comment. lmk if there is anything missing.

I meant that the new commands weren't documented in the watchtower's readme.

@sr-gi
Copy link
Member Author

sr-gi commented Aug 16, 2022

You mentioned another issue with the docs, but I couldn't see the comment. lmk if there is anything missing.

I meant that the new commands weren't documented in the watchtower's readme.

Oh, gotcha

mariocynicys
mariocynicys reviewed Aug 16, 2022
View reviewed changes
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments regarding the recent change.
Kinda putting myself in the user's chair here and see what would make more sense to them. This might be a lil opinionated though.

watchtower-plugin/src/main.rs Outdated Show resolved Hide resolved
watchtower-plugin/src/retrier.rs Outdated Show resolved Hide resolved
watchtower-plugin/src/retrier.rs Outdated Show resolved Hide resolved
@sr-gi sr-gi force-pushed the cln-plugin-additional-commands branch from be0c671 to cca0dcf Compare August 16, 2022 13:27
@sr-gi
Copy link
Member Author

sr-gi commented Aug 16, 2022

Comments have been addressed (except for the docs, which need #86 to be merged or they'll conflict).

@sr-gi sr-gi removed the Failing on CI :( label Aug 16, 2022
@sr-gi sr-gi force-pushed the cln-plugin-additional-commands branch from cca0dcf to b6a123e Compare August 17, 2022 08:28
@sr-gi
Adds some extra commands to the cln-plugin
33148c1 
- `abandontower <tower_id>` will remove all data associated to a given tower
- `getappointmentreceipt <tower_id> <locator>` will pull a given appointment receipt
from the local database.
- `getregistrationreceipt <tower_id>` will pull a given registration receipt from
the local database.
@sr-gi sr-gi force-pushed the cln-plugin-additional-commands branch from b6a123e to f12a0ac Compare August 17, 2022 10:26
@sr-gi sr-gi requested a review from mariocynicys August 17, 2022 10:27
@sr-gi
Copy link
Member Author

sr-gi commented Aug 17, 2022

Notice this may still fail for test_unreachable_watchtower due to #84

mariocynicys
mariocynicys approved these changes Aug 20, 2022
View reviewed changes
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small nit. Otherwise, should be good to go.

watchtower-plugin/src/retrier.rs Outdated
.unwrap()
.set_tower_status(tower_id, crate::TowerStatus::TemporaryUnreachable);
{
// Not start a retry if the tower is flagged to be abandoned
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Change is flagged to be abandoned to is abandoned

@sr-gi
Fixes missing tmp_dir for watchtower-plugin
a42a5ee 
Some of the tmp paths in the `watchtower-plugin` tests were not using
`TempDir` and still manually removing the directories.
@sr-gi sr-gi force-pushed the cln-plugin-additional-commands branch from f12a0ac to a42a5ee Compare August 22, 2022 09:11
@sr-gi sr-gi merged commit 2e9a3c5 into talaia-labs:master Aug 22, 2022
@sr-gi sr-gi removed the Seeking Code Review review me pls label Aug 22, 2022
@sr-gi sr-gi mentioned this pull request Aug 22, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariocynicys mariocynicys mariocynicys approved these changes

Assignees
No one assigned
Labels
cln-plugin Stuff related to watchtower-plugin Failing on CI :( feature request New feature or request
Projects
None yet
Milestone
v.0.1.2
Development

Successfully merging this pull request may close these issues.
2 participants
@sr-gi @mariocynicys