some format and name scheme specs #24
Conversation
mandelsoft
force-pushed
the
work
branch
2 times, most recently
from
cd96a7d to
2d7894b
Compare
|
One question. This doesn't appear to be just a documentation change, right? Would you mind writing a description of what the code changes are? Thanks. |
|
@mandelsoft You need rebase this pull request with latest master branch. Please check. |
| There are three different technical flavors: | ||
| - `directory`: the content is store directly as a directory tree | ||
| - `tar`: the directory tree is stored in a tar archive | ||
| - `tgz`: the directory tree is stored in a zipped tar archive |
There was a problem hiding this comment.
Nowadays this is usually tar.gz, right? So we should allow for that pattern.
There was a problem hiding this comment.
This is not used as file suffix, it is just a constant value used for configuration.
The format detection always looks into the file instead of relying on a file suffix.
There was a problem hiding this comment.
I meant, the document should maybe say something like tgz|tar.gz?
| content by value. | ||
|
|
||
| In a dedicated context all used access methods must be known by the used tool | ||
| set. Nevertheless, the set of access methods is not fixed. The actual |
There was a problem hiding this comment.
The actual
library/tool version provides a simple way to locally add new methods with
their implementations to support own local environments.
This should make it a bit clearer that this is achieved via concrete local code modification and not by any plug-in capabilities.
There was a problem hiding this comment.
It would also be nice to include some kind of example on how to extend access methods. Because if I read this as somebody new coming to the project, it would be difficult to see how and where to add new things too. Maybe include a very simple, dummy implementation of an artefact access method that downloads something from an arbitrary URL.
It's especially difficult to ascertain from this how said action will be configured then because it's not documented that a struct needs to have certain values in order for them to be parsed into it via reflection.
There was a problem hiding this comment.
Though some of this is explained below in vague terms.
There was a problem hiding this comment.
An external plugin mechanism just providing a new executable would be fine. Do you have a good idea how to achieve this,
the problem is basically the question how to pass the context, credentials as well as the involved target repository. The blob could just be passed as file.
There was a problem hiding this comment.
We could potentially use something like the go-plugin from HashiCorp, although that would bring in lots of complexity. But it's how most of the things in HashiCorp work.
Also, we could use any executable as long as it follows some conventions. For example, consider the following.
Extract the serialisation/deserialisation code into a separate library called ocm-parser. Once that's done, establish the convention for plugins that the first and second command line argument is a byte stream of serialised context and credentials. Then, they can take the ocm-parser library, deserialise the context and creds, and do whatever they want.
This way, a plugin can be anything. We could also use Go's plugin system. This gives us a bit of leeway in that it has a strict contract on how it needs to look. Has the added benefit that we can use functions with specific parameters. So a plugin function could look something like this:
Run(ctx ocmContext.Context, creds ocmCreds.Credentials) error would just return an error on a failure and do its thing otherwise. Potentially, it could produce something else as well, depending on the need for an output of some kind.
|
|
||
| ### Specification Versions | ||
|
|
||
| #### Version `v1` |
|
comments up to now fixed.... |
mandelsoft
force-pushed
the
work
branch
2 times, most recently
from
87b3fcb to
527e7f6
Compare
|
rebased to latest master |
|
@mandelsoft You need rebase this pull request with latest master branch. Please check. |
|
@mandelsoft hi! Could we merge this please and not further proliferate it with changes? :) This is getting rather large and I think I already have a merge conflict with this branch. |
|
What were the added changes? There were 3000 or something and now it's 5000.. |
|
sorry pushed to wrong branch, just reverted it |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: