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

fix: exclude config from default output in gateway-cli info rpc #5070

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: exclude config from default output in gateway-cli info rpc #5070

wants to merge 1 commit into from

Conversation

jrakibi
Copy link

@jrakibi jrakibi commented Apr 23, 2024

This PR addresses the feedback from issue #4365 by modifying the gateway-cli info command to exclude the config section from the default output.

Changes:

  • The config section is now hidden by default to prevent information overload and enhance the command's usability.
  • Users can still view the config details by using an --include-config

@jrakibi jrakibi requested review from a team as code owners April 23, 2024 16:05
@m1sterc001guy
Copy link
Contributor

This will unfortunately break backwards compatibility. To maintain backwards compatibility, you will need to change the include_config flag to be exclude_config so that by default the config is included. This way, old clients can continue to make the RPC as they did before, but new clients can add the exclude_config flag to have a more readable output.

The devimint tests will need to be updated. Then after a major release, we can invert the flag (or remove it entirely) so that its excluded by default.

@jrakibi
Copy link
Author

jrakibi commented Apr 23, 2024

@m1sterc001guy, done. I updated the flag from include_config to exclude_config, but it seems that the job for backwards-compatibility is still failing

dpc
dpc previously approved these changes Apr 23, 2024
View reviewed changes
@m1sterc001guy
Copy link
Contributor

We need to get to the bottom of the backwards compatibility failures. Is there a default value for exclude_config? If there's no default value, I would assume that old CLI versions will fail since they do no understand the exclude_config flag.

@dpc
Copy link
Contributor

dpc commented Apr 24, 2024

BTW. Probably good idea to make this controlled via env var (optionally), so in scritpts and for backward compat it can be set once for all invocations.

@elsirion
Copy link
Contributor

old CLI versions will fail since they do no understand the exclude_config flag.

Old clients won't send the exclude flag, so new servers need a default. New clients may send the exclude flag, but old servers should ignore it due to JSON.

@jrakibi jrakibi requested a review from dpc May 9, 2024 15:33
@jrakibi
Copy link
Author

jrakibi commented May 9, 2024

The exclude_config parameter now defaults to true and is optional. However, the backward compatibility job is still failing. I'm not sure if this is related or not

@m1sterc001guy
Copy link
Contributor

Try rebasing and let's re-run the tests and see if it is still broken

@jrakibi jrakibi force-pushed the 2024-04-improve-rpc-cmd branch 2 times, most recently from 710c754 to c8cff66 Compare May 14, 2024 19:52
@jrakibi
Copy link
Author

jrakibi commented May 14, 2024 •
edited

Try rebasing and let's re-run the tests and see if it is still broken

@m1sterc001guy Done

m1sterc001guy
m1sterc001guy reviewed May 14, 2024
View reviewed changes
gateway/cli/src/main.rs Outdated
@@ -155,6 +163,9 @@ pub enum LightningCommands {
/// The delay between retries
#[clap(long)]
retry_delay_seconds: Option<u64>,

#[clap(long)]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can use default_value in clap

m1sterc001guy
m1sterc001guy reviewed May 14, 2024
View reviewed changes
gateway/cli/src/main.rs Outdated
@@ -324,7 +350,9 @@ async fn main() -> anyhow::Result<()> {
block_height,
max_retries,
retry_delay_seconds,
exclude_config,
Copy link
Contributor

Choose a reason for hiding this comment

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

You're just throwing away the config below anyway since the only field that is used is block_height, so this doesn't need to be a parameter. Just set it to true in the actual get_info RPC

m1sterc001guy
m1sterc001guy reviewed May 14, 2024
View reviewed changes
gateway/ln-gateway/src/rpc/mod.rs Outdated
@@ -65,7 +68,8 @@ pub struct WithdrawPayload {
pub struct FederationInfo {
pub federation_id: FederationId,
pub balance_msat: Amount,
pub config: ClientConfig,
#[serde(skip_serializing_if = "Option::is_none")] // Skip serializing if config is None
pub config: Option<ClientConfig>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so the backwards compatibility tests might be failing because this changed from ClientConfig to Option<ClientConfig> (havent confirmed myself yet).

I don't think changing this is actually necessary. The main issue with the ClientConfig is just that it clutters up the CLI response. I think we should still return ClientConfig on the get_info requests, but on the CLI just filter out the config according to the exclude_config flag. This will make it so the CLI is more readable but also keep the change simple on the API/server side.

Copy link
Author

Choose a reason for hiding this comment

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

Oh perfect, sounds like a straightforward solution then. We will just update one file.

I've made the changes and updated the PR. Can you please review it again and run the jobs to check if the backward compatibility issue still occurs?

@jrakibi
fix: exclude config from default output in gateway-cli info rpc
a37f2a8 
refactor: Simplify payload structure in integration tests (redundant fieldname)

fix: change flag from include-config to exclude-config to prevent backward compatibility erros

fix: fix backward compatibility issue

fix: Trigger CI

fix: exclude config from default output in gateway-cli info rpc

fix: exclude config from default output in gateway-cli info rpc
@elsirion
Copy link
Contributor

Somewhere in the tests we assume that the config field is present, needs to be changed for this PR. https://github.com/fedimint/fedimint/actions/runs/9142510136/job/25229423380?pr=5070#step:9:250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpc dpc Awaiting requested review from dpc

@m1sterc001guy m1sterc001guy Awaiting requested review from m1sterc001guy

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jrakibi @m1sterc001guy @dpc @elsirion