-
Notifications
You must be signed in to change notification settings - Fork 22
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 --lookup option #65
Conversation
return nil | ||
} | ||
|
||
func WithResolver(resolver ocm.ComponentVersionResolver) transferhandler.TransferOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The options functions so far do not is the With prefix.
Such functions are typically used to setup a builder, which we do not have here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
}) | ||
}) | ||
|
||
env.OCMCommonTransport(componentBArchive, accessio.FormatDirectory, func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really additive, if A and B are the same archive file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i debugged into a test and it works.
also the other test is still running
} | ||
|
||
func (o *resolverOption) ApplyTransferOption(to transferhandler.TransferOptions) error { | ||
to.(ResolverOption).SetResolver(o.resolver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be, we should for all those cases return an error if the actual option set does not accept this kind of config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@jschicktanz You need rebase this pull request with latest master branch. Please check. |
What this PR does / why we need it:
add
--lookup
option to various commands.as a prerequisite, added support for lookup option in transfer handler.
Which issue(s) this PR fixes:
Fixes #17
Special notes for your reviewer:
Release note: