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

Add new ResolverWithOpts #5

Merged
merged 5 commits into from
Jun 7, 2021

Conversation

jdolitsky
Copy link
Contributor

@jdolitsky jdolitsky commented Jun 7, 2021

This will allow us to add new options to Resolver without breaking API in the future.

In the near term, this allows us to set a custom User-Agent header:

headers := http.Header{}
headers.Set( "User-Agent", "my-user-agent")
resolver := ResolverWithOpts(WithResolverHeaders(headers))

Resolves oras-project/oras#240

jdolitsky added 2 commits June 7, 2021 10:41
Signed-off-by: Josh Dolitsky <josh@dolit.ski>
Signed-off-by: Josh Dolitsky <josh@dolit.ski>
@jdolitsky jdolitsky requested a review from deitch June 7, 2021 14:47
jdolitsky added 2 commits June 7, 2021 10:50
Signed-off-by: Josh Dolitsky <josh@dolit.ski>
Signed-off-by: Josh Dolitsky <josh@dolit.ski>
Copy link
Contributor

@deitch deitch left a comment

Choose a reason for hiding this comment

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

A few things I noticed.

  • We used to pass a context.Context to Resolver(). Do we not need it anymore?
  • In other areas, functions to set options passed as variadic parameters always were named With*, which would make these WithResolverOptClient(client *http.Client) or WithResolverOptPlainHTTP(). Should we be consistent in the naming?
  • whether or not to use plain http is a boolean, which defaults to false. If the only thing we will do is set it to true, then it probably should just be ResolverOptPlainHTTP() (or WithResolverOptPlainHTTP(), not parameter required

Right now, just a comment.

Signed-off-by: Josh Dolitsky <josh@dolit.ski>
@jdolitsky
Copy link
Contributor Author

@deitch -

We used to pass a context.Context to Resolver(). Do we not need it anymore?

It's actually not needed. The original implementation was just throwing it away (Resolver(_ context.Context, ...). The beauty of this is that if it is needed later, we can add a new WithResolverContext(context.Context) option.

In other areas, functions to set options passed as variadic parameters always were named With*, which would make these WithResolverOptClient(client *http.Client) or WithResolverOptPlainHTTP(). Should we be consistent in the naming?

Yes, agree. Done.

whether or not to use plain http is a boolean, which defaults to false. If the only thing we will do is set it to true, then it probably should just be ResolverOptPlainHTTP() (or WithResolverOptPlainHTTP(), not parameter required

Done.

@jdolitsky jdolitsky requested a review from deitch June 7, 2021 15:33
@jdolitsky jdolitsky merged commit 933c6d5 into oras-project:main Jun 7, 2021
@jdolitsky jdolitsky mentioned this pull request Jun 7, 2021
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.

Permit user-agent configuration
2 participants