-
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
enable adding (re)sources directly via command line options #161
Conversation
This is a replacement for #154, which was corrupted after setting this repo to pubic. |
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.
A few questions, notes, suggestions.
"github.com/open-component-model/ocm/pkg/contexts/ocm/core" | ||
"github.com/open-component-model/ocm/pkg/runtime" | ||
) | ||
|
||
func _handler(handler []flagsets.ConfigOptionTypeSetHandler) flagsets.ConfigOptionTypeSetHandler { |
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.
Why does it have an underscore at the beginning of the function, it's not python, it's already private (not exported) as it starts with a lower case letter.
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.
It starts with an _ because it is just an short internal helper function to determine the described handler. It didn't want to create an artificial name (!= handler), therefore I just prefixed it with an underscore.
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.
prefixing with _
does not look something Go uses in general. Can we prefix it with something, even something like localHandler
, selectHandler
, or basically anything without _
😆 as I see it does only one thing: pick the first handler in the list, or nil if the list is empty.
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.
I see a common pattern about this, we can make a function for that with generics:
func pickFirst[T any](items []*T) *T {
if len(items) > 0 {
return items[0]
}
return nil
}
This function can be used with any type of lists, I remember we have a few other places with the same functionality.
Not now, but long term. From Go 1.18 generics are available and this is a good candidate to remove a few other functions with the exact same logic.
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.
good idea, I'll put this into the utils package later
What this PR does / why we need it:
As an alternative to adding resources/sources by specifications in a separate source file now three options can be used
to completely describe a single (re)source via command line options (#144):
--resource
/--source
/--reference
: to describe the meta data--input
: to describe the data used to compose a resource stored locally with the component version--access
: to describe the access specification for referenced artifactsThe data for the meta data is partly fixed, but potentially very complex, therefore it does not seem to be appropriate to describe these attributes by dedicated options. The data for the input and access highly depends on the structure of the used types. The command just uses a dynamic deserialization as part of the currently used specification file and a validation provided by registered implementations. Therefore it is not really possible to find dedicated options for all the possible variants.
Because of this, all three options accept a json/yaml given as string. Only this allows specifying the required arbitrary and potentially deep data structures.
Additionally, there are dedicated options for the meta data, like
--name
,--version
, and--type
for (re) sources and--component
for references. This allows specifying basic meta data with no labels or extra identities just by command line options. Both variants can be combined: specify required meta data per dedicated value option and the rest optionally via the YAML based option.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
To handle the dynamic nature of the configuration of access method and input specifications required for the artifact composition commands a new type of option handling has been introduced. With OptionTypes, OptionTypeSets and ConfigProviders based on OptionTypes is it possible now for dynamically registered elements to compose options sets and to validate given option sets and finally map given values to generic configuration objects.
This feature is then used by the dynamic parts to register appropriate handlers used by the CLI commands.
They now are completely based on this mechanism to handle meta data of the various element types (resources, sources, references) to be added to a component version, and the content offered either by inputs or access specifications.
Additionally, commands based on the same mechanism has been introduced to configure appropriate
element specification source files (resources.yaml, sources.yaml), which can then be used as traditional control files for the (component version) add commands.
Release note:
It is possible now to describe artifacts or references to be added to a component version completely or partly
by options of the
add
commands without the use of an element description file.For example:
Additionally there are commands accepting the same options to compose an element description file usable for the
add commands:
$ ocm add resourceconfig resources.yaml --name myresource --type PlainText --input '{ "type": "file", "path": "testdata/testcontent", "mediaType": "text/plain" }'