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

feat(client_cli): Integrating CLI API to set and retrieve parameters. #4961

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

divyaranjan1905
Copy link

@divyaranjan1905 divyaranjan1905 commented Aug 11, 2024

Description

This PR would integrate the necessary patchwork needed to ensure that one could use the client_cli of iroha to perform operations to set new values for parameters and retrieve existing ones.

Linked issue

Closes #4820

Benefits

This creates an easier way for the user to modify parameter values.

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

Copy link

github-actions bot commented Aug 11, 2024

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification, and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Initial commit for parameters cli module.

Signed-off-by: Divya Ranjan <divya@subvertising.org>". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@divyaranjan1905 divyaranjan1905 changed the title Integrating CLI API to set and retrieve parameters. feat(client_cli): Integrating CLI API to set and retrieve parameters. Aug 11, 2024
@divyaranjan1905
Copy link
Author

divyaranjan1905 commented Aug 11, 2024

Okay, so @nxsaken and others. This draft PR might have several errors and the code might not be the best, bear with me as I fix these things.

Comment on lines +1099 to +1145
impl RunArgs for Get {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
let client = context.client_from_config();
let vec = match self {
Self::All => client
.request(client::parameter::all())
.wrap_err("Failed to get all the parameters."),
Self::Inner(param) => client
.build_query(client::parameter::all())
.with_filter(ParamCmd::Sumeragi)
.execute()
.wrap_err("Failed to get the parameter value"),
}?;
context.print_data(&vec.collect::<QueryResult<Vec<_>>>()?)?;
Ok(())
}
}
Copy link
Author

Choose a reason for hiding this comment

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

So the first thing is here, I tried to replicate this from another commit, but I'm stuck on how to get the specific parameters that are in ParamCmd. I'd like some suggestion on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

client::parameter::all() returns Parameters, which has all the parameters as fields. No need to filter here. You need to map ParamCmd to the correct field in Parameters

Copy link
Contributor

Choose a reason for hiding this comment

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

You will also need to make a version of ParamCmd and its nested types without the value field for this.

Copy link
Author

Choose a reason for hiding this comment

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

You will also need to make a version of ParamCmd and its nested types without the value field for this.

@nxsaken I would need to duplicate it again? That seems weird. Maybe ParamCmd could be refactored for the better?

Comment on lines +1125 to +1161
impl RunArgs for Set {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
let set_param_value = SetParameter::new(0);
// Not sure what to do.
Ok(())
}
}
Copy link
Author

Choose a reason for hiding this comment

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

And similarly, how do we exactly take the parameters from the hierarchy we have and actually set them? I believe we need to use SetParameter from data_model::parameter, but don't know how to go further on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

SetParameter is an Instruction, which can be submitted to an Iroha Client. You need to turn self into a SetParameter here, and then call submit (defined in this file).

client_cli/src/main.rs Show resolved Hide resolved
Comment on lines +1099 to +1145
impl RunArgs for Get {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
let client = context.client_from_config();
let vec = match self {
Self::All => client
.request(client::parameter::all())
.wrap_err("Failed to get all the parameters."),
Self::Inner(param) => client
.build_query(client::parameter::all())
.with_filter(ParamCmd::Sumeragi)
.execute()
.wrap_err("Failed to get the parameter value"),
}?;
context.print_data(&vec.collect::<QueryResult<Vec<_>>>()?)?;
Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

client::parameter::all() returns Parameters, which has all the parameters as fields. No need to filter here. You need to map ParamCmd to the correct field in Parameters

Comment on lines +1125 to +1161
impl RunArgs for Set {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
let set_param_value = SetParameter::new(0);
// Not sure what to do.
Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

SetParameter is an Instruction, which can be submitted to an Iroha Client. You need to turn self into a SetParameter here, and then call submit (defined in this file).

Comment on lines +1099 to +1145
impl RunArgs for Get {
fn run(self, context: &mut dyn RunContext) -> Result<()> {
let client = context.client_from_config();
let vec = match self {
Self::All => client
.request(client::parameter::all())
.wrap_err("Failed to get all the parameters."),
Self::Inner(param) => client
.build_query(client::parameter::all())
.with_filter(ParamCmd::Sumeragi)
.execute()
.wrap_err("Failed to get the parameter value"),
}?;
context.print_data(&vec.collect::<QueryResult<Vec<_>>>()?)?;
Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You will also need to make a version of ParamCmd and its nested types without the value field for this.

Signed-off-by: Divya Ranjan <divya@subvertising.org>
@mversic
Copy link
Contributor

mversic commented Sep 26, 2024

@nxsaken what's the state with this?

@nxsaken
Copy link
Contributor

nxsaken commented Sep 26, 2024

@divyaranjan1905 hi, do you plan to finish this PR?

@divyaranjan1905
Copy link
Author

I do, I've simply been on a hiatus due to some stuff. I plan to get back on this within a week or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API to set parameters through CLI
3 participants