Skip to content

feat: canister metadata command#2335

Merged
mergify[bot] merged 4 commits intomasterfrom
ericswanson/sdk-346-canister-metadata
Jul 15, 2022
Merged

feat: canister metadata command#2335
mergify[bot] merged 4 commits intomasterfrom
ericswanson/sdk-346-canister-metadata

Conversation

@ghost
Copy link

@ghost ghost commented Jul 11, 2022

Description

Adds dfx canister metadata <canister> <metadata name> command, for example dfx canister metadata something_backend candid:service to display the candid service definition for the backend canister.

Open questions:

  1. In addition to general utility, this is intended to aid deprecation of the /_/candid endpoint. That endpoint, however, has options for formatting the candid definition in either .js or .ts format. Should this command have some option to perform that transformation, or is there some other tool that is better suited to the purpose? What is an example of a consumer of that functionality?

Fixes https://dfinity.atlassian.net/browse/SDK-346

How Has This Been Tested?

Added e2e tests

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@chenyan-dfinity
Copy link
Contributor

Should this command have some option to perform that transformation, or is there some other tool that is better suited to the purpose? What is an example of a consumer of that functionality?

Either Candid UI or didc can handle the transformation. The metadata command should not handle the transformation as it's too specific to the Candid metadata. Also the _/candid transformation is only used by Candid UI, which is deprecated now already.

.await
.with_context(|| format!("Failed to read controllers of canister {}.", canister_id))?;

stdout().write_all(&metadata)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure the best way to display this. Some of the metadata may not be printable ascii. Maybe we can check if metadata is printable, if not, we display the hex instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have an --outfile flag instead/in addition?

Copy link
Author

@ghost ghost Jul 12, 2022

Choose a reason for hiding this comment

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

@chenyan-dfinity @sesi200 I wondered about these too. Not sure of the best way either.

The thinking was that by writing the raw data bytes to stdout, they can be written to a file or piped to another program.

While the metadata may not be printable ascii, what if the developer wants to run the actual contents through hexdump or some other tool? If dfx canister metadata ... | hexdump -C, and the metadata is not entirely printable ascii, what would the expected behavior be?

What would the proposed rules be? Would they more clear than always writing as-is to stdout? The below seem overly complicated to me.

  • if --outfile specified, write data to specified file as-is. --outfile - means write data as-is to stdout.
  • If no --outfile specified, and metadata is printable ascii, write that to stdout.
  • If no --outfile specified, and metadata is not all printable ascii, then write an explanation to stderr and display metadata as hex to stdout.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no strong opinion either way and have no problem merging this as-is. I was mostly thinking about invoking dfx from somewhere that doesn't have such trivial redirect mechanics, but I might just be overthinking it.

My approach would have been to print to --outfile if specified, otherwise print to stdout. Reason being that I'd want to give a way to not see garbage printed if it is expected.

@ghost ghost marked this pull request as ready for review July 15, 2022 01:15
@ghost ghost self-requested a review as a code owner July 15, 2022 01:15
use ic_types::Principal;
use std::io::{stdout, Write};

/// Sign a canister call and generate message file.
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
/// Sign a canister call and generate message file.
/// Displays metadata in a canister.

Copy link
Contributor

Choose a reason for hiding this comment

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

I went ahead and merged this already

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for noticing this!

@ghost ghost added the automerge-squash label Jul 15, 2022
@mergify mergify bot merged commit af2bb39 into master Jul 15, 2022
@mergify mergify bot deleted the ericswanson/sdk-346-canister-metadata branch July 15, 2022 17:13
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.

2 participants