Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

Added par support to wash-lib #844

Merged
merged 3 commits into from
Oct 3, 2023

Conversation

aish-where-ya
Copy link
Contributor

Feature or Problem

Feat: Move wash par to wash-lib

Release Information

next

Consumer Impact

Internal code refactoring.
No impact to existing functionality

Testing

unit and manual tests

Built on platform(s)

  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
  • x86_64-windows

Tested on platform(s)

  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
  • x86_64-windows

Unit Test(s)

NA

Acceptance or Integration

NA

Manual Verification

manually verified

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

One nit, one comment that may not be applicable anymore, but mostly ergonomic changes that feel like they would make the functions here a whole lot simpler if we pushed a little responsibility on the CLI-side. Essentially, I think finding and loading keypairs and files should be the responsibility of the CLI, and the library can just properly take that information and turn it into the appropriate ProviderArchive structs that the CLI can then write out

crates/wash-lib/src/cli/par.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/par.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/par.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/par.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/par.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/par.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/par.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/par.rs Outdated Show resolved Hide resolved
crates/wash-lib/src/cli/par.rs Outdated Show resolved Hide resolved
@aish-where-ya
Copy link
Contributor Author

I think I managed to address all the review comments here. Pulling stuff out from the library into the CLI definitely helped in reducing some of the function arguments and also removing the function argument struct for the insert_provider_binary, thus reducing a lot of clutter.

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

Looks really clean 👍🏻 One tiny request to rename function but looks good to me

Ok(par)
}

pub async fn insert_provider_archive(
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be named insert_provider_binary to be specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oopsies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: aish-where-ya <[email protected]>
@brooksmtownsend brooksmtownsend merged commit 90f7944 into wasmCloud:main Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants