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

Parse ChangeHistory command-line arguments #8365

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

Nathy-bajo
Copy link
Contributor

@Nathy-bajo Nathy-bajo commented Jul 30, 2024

Current behavior

Currently, the ockam_command has multiple places where Identity change history is accepted as a hex-encoded string. This approach lacks type safety and does not validate or decode the input into a structured format.

Proposed changes

Introduced a new crate, ockam_common, which includes the ValueParser struct.

ValueParser is designed to validate and decode hex-encoded strings into structured data representing Identity change history.

Implemented a ChangeHistoryParser which utilizes the ValueParser for argument parsing in CLI commands. This ensures that the inputs are validated and decoded properly before being used.

Checks

  • All commits in this Pull Request are signed and Verified by Github.
  • All commits in this Pull Request follow the Ockam commit message convention.
  • There are no Merge commits in this Pull Request. Ockam repo maintains a linear commit history. We merge Pull Requests by rebasing them onto the develop branch. Rebasing to the latest develop branch and force pushing to your Pull Request branch is okay.
  • I have read and accept the Ockam Community Code of Conduct.
  • I have read and accepted the Ockam Contributor License Agreement by adding my Git/Github details in a row at the end of the CONTRIBUTORS.csv file in a separate pull request to the build-trust/ockam repository. The easiest way to do this is to edit the CONTRIBUTORS.csv file in the github web UI and create a separate Pull Request, this will mark the commit as verified.

@etorreborre
Copy link
Member

Hi @Nathy-bajo I am going to review the PR today. In the meantime can you please have a look at the list of checks in the PR description? In particular you need to format your commits and open a separate PR to be added to the contributors file. Thanks.

@Nathy-bajo
Copy link
Contributor Author

Hi @Nathy-bajo I am going to review the PR today. In the meantime can you please have a look at the list of checks in the PR description? In particular you need to format your commits and open a separate PR to be added to the contributors file. Thanks.

Alright, I will do just that.

@Nathy-bajo Nathy-bajo closed this Aug 2, 2024
@Nathy-bajo Nathy-bajo reopened this Aug 2, 2024
@Nathy-bajo
Copy link
Contributor Author

@etorreborre Please can you review this PR

@etorreborre
Copy link
Member

Hi @Nathy-bajo, I think we can do something simple by leveraging some existing code without a new crate.
For example, for the authority_identity field we can declare:

#[arg(long, value_name = "ACCOUNT_AUTHORITY_CHANGE_HISTORY", default_value = None, value_parser = ChangeHistory::import_from_string)]
    account_authority: Option<ChangeHistory>,

A bit of massaging for error types could be necessary but otherwise this should work.

@Nathy-bajo
Copy link
Contributor Author

Hi @Nathy-bajo, I think we can do something simple by leveraging some existing code without a new crate. For example, for the authority_identity field we can declare:

#[arg(long, value_name = "ACCOUNT_AUTHORITY_CHANGE_HISTORY", default_value = None, value_parser = ChangeHistory::import_from_string)]
    account_authority: Option<ChangeHistory>,

A bit of massaging for error types could be necessary but otherwise this should work.

Hello @etorreborre 😄
I have updated the code as you requested.

@etorreborre
Copy link
Member

Hi @Nathy-bajo, that looks good, can you please rebase to fix the conflict?

@Nathy-bajo
Copy link
Contributor Author

Nathy-bajo commented Aug 15, 2024

Hi @Nathy-bajo, that looks good, can you please rebase to fix the conflict?

I have rebased it.
Please can you confirm?

@etorreborre
Copy link
Member

Hi @Nathy-bajo. We don't accept merge commits on our branches, only rebasing.
Can you please have a look at the CI job results, I see several issues which shouldn't be too hard too fix:

  • With your commit message. It should be something like feat(rust): ...
  • With some unused imports.
  • With code formatting.

@Nathy-bajo
Copy link
Contributor Author

Hello @etorreborre. Hope you're having a good day😄
I have fixed the issues. Is there anything else I need to do?

@etorreborre
Copy link
Member

@Nathy-bajo can you please have a look at the failed jobs? For example you will have to get rid of the merge commit 0c026b8 (one way to do this is to recreate your branch entirely and force-push it).

@Nathy-bajo
Copy link
Contributor Author

@Nathy-bajo can you please have a look at the failed jobs? For example you will have to get rid of the merge commit 0c026b8 (one way to do this is to recreate your branch entirely and force-push it).

Hello @etorreborre
I have removed unused imports as requested in the failed jobs and formatted the code. I have also created a new branch and force-pushed it.

@Nathy-bajo
Copy link
Contributor Author

Nathy-bajo commented Aug 18, 2024

Hello @etorreborre
I have tried using git reset --soft HEAD~4 to move HEAD back to the point just before the merge commit, then rebased it and force-pushed it because the job failing is still coming from the merge commit.

@etorreborre
Copy link
Member

Let me try to fix this PR.

@etorreborre
Copy link
Member

@Nathy-bajo we're getting there!

@Nathy-bajo
Copy link
Contributor Author

@Nathy-bajo we're getting there!

Hello @etorreborre 😄
Nice work! I was even thinking of creating a new PR if this did not work.
Thank you.

@etorreborre etorreborre added this pull request to the merge queue Aug 19, 2024
Merged via the queue into build-trust:develop with commit ec0634b Aug 19, 2024
28 checks passed
@etorreborre etorreborre changed the title ockam_common added with fix Parse ChangeHistory command-line arguments Aug 20, 2024
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