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

[restatectl] Support metadata Put #2055

Merged
merged 5 commits into from
Oct 10, 2024

Conversation

igalshilman
Copy link
Contributor

@igalshilman igalshilman commented Oct 10, 2024

This commit adds support for put operations for metadata It is just a convenience around patch.

 ./target/debug/restatectl metadata get --key "partition_table" > foo.json

Either:

restatectl metadata put --key "partition_table" foo.json

Or:

cat foo.json | restatectl metadata put --key "partition_table" --path foo.json -

@igalshilman igalshilman requested a review from pcholakov October 10, 2024 15:10
This commit adds support for put operations for metadata
It is just a convenience around patch.
@AhmedSoliman
Copy link
Contributor

@igalshilman take a look at clap_stdin to allow reading the input from stdin rather than with --doc.

e.g.

cat foo.json | restatectl metadata put --key -

Copy link
Contributor

@pcholakov pcholakov left a comment

Choose a reason for hiding this comment

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

I believe this will do the trick! Thank you for helping out, @igalshilman!

Comment on lines +62 to +55
if let Some(obj) = doc.as_object_mut() {
// make sure that the value does not contain the version field.
obj.remove("version");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine, since patch will insert its own version, derived from the store-read version, on the write path.

@igalshilman
Copy link
Contributor Author

igalshilman commented Oct 10, 2024

@igalshilman take a look at clap_stdin to allow reading the input from stdin rather than with --doc.

e.g.

cat foo.json | restatectl metadata put --key -

@AhmedSoliman ha nice, i didn't know about that.
just to make sure, you mean instead of --path (you wrote --doc) ?
or would rather support only stdin?

@AhmedSoliman
Copy link
Contributor

@igalshilman take a look at clap_stdin to allow reading the input from stdin rather than with --doc.
e.g.

cat foo.json | restatectl metadata put --key -

@AhmedSoliman ha nice, i didn't know about that. just to make sure, you mean instead of --path (you wrote --doc) ? or would rather support only stdin?

it'd be a file path or through stdin, for instance

restatectl metadata put -k "partition_table" partition_table.json

or

echo "{"bad_json": true}" | restate metadata put -k partition_table -

or

cat partition_table.json | metadata put -k partition_table -

So it replaces --path and --doc. Check out FileOrStdin in that crate

Comment on lines 11 to 18
use crate::commands::metadata::patch::{patch_value, PatchValueOpts};
use crate::commands::metadata::MetadataCommonOpts;
use anyhow::anyhow;
use clap::Parser;
use clap_stdin::FileOrStdin;
use cling::{Collect, Run};
use serde_json::Value;
use std::collections::HashMap;
Copy link
Contributor

@AhmedSoliman AhmedSoliman Oct 10, 2024

Choose a reason for hiding this comment

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

nit: Our convention is to split imports into

<std>
blank
<third-party>
blank
<restate_*>
blank
<crate/super/self::*>

Comment on lines 49 to 50
let mut doc: Value = serde_json::from_str(&doc_body)
.map_err(|e| anyhow::anyhow!("Parsing JSON value: {}", e))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut doc: Value = serde_json::from_str(&doc_body)
.map_err(|e| anyhow::anyhow!("Parsing JSON value: {}", e))?;
let mut doc: Value = serde_json::from_str(&doc_body).context("Failed to parse JSON value")?;

Although I don't think we need either solutions.

patch_command.insert("value", doc);

let patch = vec![patch_command];
let patch_as_str = serde_json::to_string_pretty(&patch).map_err(|e| anyhow::anyhow!(e))?;
Copy link
Contributor

@AhmedSoliman AhmedSoliman Oct 10, 2024

Choose a reason for hiding this comment

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

You shouldn't need to map_err in this case.

Copy link
Contributor

@AhmedSoliman AhmedSoliman left a comment

Choose a reason for hiding this comment

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

❤️

@igalshilman
Copy link
Contributor Author

Thanks for the review @AhmedSoliman and @pcholakov,
will merge once green.

@AhmedSoliman AhmedSoliman merged commit fc12aa8 into restatedev:main Oct 10, 2024
10 checks passed
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.

3 participants