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

Allow parcel ref in component source #456

Merged
merged 1 commit into from
May 10, 2022

Conversation

itowlson
Copy link
Contributor

@itowlson itowlson commented May 9, 2022

Fixes #454.

This duplicates the BindleConnectioninfo infrastructure from the Hippo CLI and the bindle push command. It looks like the publish crate depends on loader so we can probably remove it from publish, though it feels a bit weird for loader to be the one to export it - it's not the obvious place a consumer would look for it. We should discuss where we want things like this to live.

It may be that there's a nicer way of threading the connection info through the loader - very much open to suggestions here.

TO DO NOW DONE:

  • Write some tests
  • Beef up the error handling

@lann
Copy link
Collaborator

lann commented May 9, 2022

In the longer term - if we wanted loaders and publishers to be pluggable - all the bindle code could move into a spin-bindle crate that provides both of those implementations.

Comment on lines +25 to +26
username: Option<String>,
password: Option<String>,
Copy link
Collaborator

@lann lann May 9, 2022

Choose a reason for hiding this comment

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

How about something like this, to allow easier extension to other auth types:

pub fn basic_auth(&mut self, username: String, password: String) ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see this is copied from publish, so this doesn't need to be in scope for this PR.

/// Returns a client based on this instance's configuration
pub fn client(&self) -> bindle::client::Result<Client<AnyAuth>> {
let builder = ClientBuilder::default()
.http2_prior_knowledge(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be the default...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think when this code was written it was not the default (in fact I think I had to add the option so I could set it to false), so this is historical I guess... but unless the default is contractual (that is, it would be documented as a breaking change if they changed it), I'm inclined to set this explicitly. (Ultimately it should be under config control.)

@itowlson itowlson force-pushed the wasm-bindle-parcel-source branch 3 times, most recently from 8f89076 to f270293 Compare May 10, 2022 01:01
@itowlson itowlson marked this pull request as ready for review May 10, 2022 01:01
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
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.

Implement Bindle module source in application manifest
2 participants