Skip to content

plugin: Added .params.configuration to init call - #2141

Merged
cdecker merged 1 commit into
masterfrom
plugin-params-configuration
Dec 7, 2018
Merged

plugin: Added .params.configuration to init call#2141
cdecker merged 1 commit into
masterfrom
plugin-params-configuration

Conversation

@cdecker

@cdecker cdecker commented Dec 6, 2018

Copy link
Copy Markdown
Member

This tells the plugin both the lightning-dir as well as the
rpc-filename to use to talk to lightningd. Prior to this they'd
had to guess.

This was supposed to be in #2131, but I had it stashed and forgot about it 🙁

@cdecker cdecker added the plugin label Dec 6, 2018
@cdecker cdecker added this to the v0.6.3 milestone Dec 6, 2018
@cdecker cdecker self-assigned this Dec 6, 2018

@renepickhardt renepickhardt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

as far as I understand the plugin architecture this patch looks good fore me. Once this is merged I will also update #2139 to utilize this right away

@renepickhardt

Copy link
Copy Markdown
Collaborator

tested your code with my updated plugin and it worked like a charm. how should I push my code? just the plugin your PR merged in my PR?

@renepickhardt

Copy link
Copy Markdown
Collaborator

I have merged this PR into #2139 because the proposed plugin depends on these changes and they turned out to work well in practice. While I am happy with this PR I suggest to close this PR here and merge #2139 in order to avoid merging conflicts on master. Otherwise I can also change my PR with the plugin by just adding the python file.

Comment thread lightningd/plugin.c Outdated
/* Add .params.configuration */
json_object_start(req->stream, "configuration");
json_add_string(req->stream, "lightning-dir", ld->config_dir);
json_add_string(req->stream, "rpc-filename", ld->rpc_filename);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The command-line option is called 'rpc-file', so let's match that.

Comment thread lightningd/plugin.c
json_object_end(req->stream); /* end of .params.options */

/* Add .params.configuration */
json_object_start(req->stream, "configuration");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"dir", "params", "configuration".... One of these is not like the other :)

Comment thread lightningd/plugin.c

struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book,
struct jsonrpc *rpc)
struct jsonrpc *rpc, struct lightningd *ld)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe should remove log_book and rpc here, and replace by ld->log_book and log->rpc in follow.

@cdecker

cdecker commented Dec 7, 2018

Copy link
Copy Markdown
Member Author

ACK ef11628

@cdecker
cdecker force-pushed the plugin-params-configuration branch 2 times, most recently from 885773c to 1930920 Compare December 7, 2018 15:08
This tells the plugin both the `lightning-dir` as well as the
`rpc-filename` to use to talk to `lightningd`. Prior to this they'd
had to guess.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
@cdecker
cdecker force-pushed the plugin-params-configuration branch from 1930920 to ef11628 Compare December 7, 2018 15:10
@cdecker
cdecker merged commit be7674e into master Dec 7, 2018
@rustyrussell
rustyrussell deleted the plugin-params-configuration branch July 22, 2020 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants