Mandelsoft/fix #75
Mandelsoft/fix #75
Conversation
mandelsoft
requested review from
achimweigel and
Diaphteiros
and removed request for
a team
|
Thank you for extracting it! :) |
| }, nil | ||
| } | ||
|
|
||
| func (a *action) Add(e interface{}) error { | ||
| if a.count > 0 && !a.Ref.IsRegistry() { | ||
| return fmt.Errorf("cannot copy multiple source artefacts to the same artefact (%s)", &a.Ref) | ||
| src := e.(*artefacthdlr.Object) |
There was a problem hiding this comment.
What happens if it fails to convert/cast type? An ok check would be useful here:
src, ok := e.(*artefacthdlr.Object)
if !ok {
// return some error
// or recover somehow
}
There was a problem hiding this comment.
Then it panics, This seems to be useful because then the complete code is wrong. The handler instantiated above declares this Object to be its implementation of the selected objects. The action now just processes the elements provided by this handler, and the contract between handler and action is this object type.
This does not appear in a dedicated case based on some input, but always.
There was a problem hiding this comment.
Now I see it's under cmd/ where it's totally fine to crash when something unexpected happens. Does not look nice to the user, but it does not cause pain to other devs.
| func (b *fileBuffer) Close() error { | ||
| err := b.file.Close() | ||
| err2 := os.Remove(b.path) | ||
| if err2 != nil { | ||
| return err2 | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
| func (b *fileBuffer) Close() error { | |
| err := b.file.Close() | |
| err2 := os.Remove(b.path) | |
| if err2 != nil { | |
| return err2 | |
| } | |
| return err | |
| } | |
| func (b *fileBuffer) Close() error { | |
| if err := b.file.Close(); err != nil { | |
| return err | |
| } | |
| return os.Remove(b.path) | |
| } |
There was a problem hiding this comment.
That is different behavior! But I'll do it differently....
There was a problem hiding this comment.
I see, so don't remove the file in the close method. :)
| type NamespaceLister struct{} | ||
|
|
||
| var anonymous cpi.NamespaceLister = &NamespaceLister{} | ||
|
|
||
| func (n *NamespaceLister) NumNamespaces(prefix string) (int, error) { | ||
| if prefix == "" { | ||
| return 1, nil | ||
| } | ||
| return 0, nil | ||
| } | ||
|
|
||
| func (n *NamespaceLister) GetNamespaces(prefix string, closure bool) ([]string, error) { | ||
| if prefix == "" { | ||
| return []string{""}, nil | ||
| } | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Sorry, I tried. But I fail to understand what this is / does.
Also, GetNamsepaves doesn't appear to be using its closure value. These two are never called in this entire changeset. So I assume it must be working with some existing code?
There was a problem hiding this comment.
The GetNamespaces function signature comes from this interface:
// NamespaceLister provides the optional repository list functionality of
// a repository
type NamespaceLister interface {
// NumNamespaces returns the number of namespaces found for a prefix
// If the given prefix does not end with a /, a repository with the
// prefix name is included
NumNamespaces(prefix string) (int, error)
// GetNamespaces returns the name of namespaces found for a prefix
// If the given prefix does not end with a /, a repository with the
// prefix name is included
GetNamespaces(prefix string, closure bool) ([]string, error)
}
There was a problem hiding this comment.
I know where it comes from. :D I was just wondering what it's doing and maybe it's not using it by accident. :) ¯\_(ツ)_/¯
There was a problem hiding this comment.
An artifact set only has an empty namespace. Therefore is returns 1, if the empty namespace or prefix is requested for the number of available namespaces (OCI repositories).
GetNamespaces()return the list with the empty namespace, if an empty prefix is requests. This is the namespace it supports. If a prefix is requested, then there are no matching namespaces hosted by the artifact set,
There was a problem hiding this comment.
I add comments, here.
| Expect(env.CatchOutput(buf).Execute("transfer", "artefact", ARCH+"//"+NS+":"+VERSION, "directory::"+OUT+"//changed")).To(Succeed()) | ||
| Expect("\n" + buf.String()).To(Equal( | ||
| ` | ||
| copying /tmp/ctf//mandelsoft/test:v1 to directory::` + OUT + `//changed:v1... |
There was a problem hiding this comment.
That's strange /tmp/ctf//mandelsoft/test:v1. Why is // good in this case? That seems like an error.
There was a problem hiding this comment.
/tmp/ctf ithe file path of the CTF archive, mandelsoft/test:v1 is the repository+version of the OCI artefact to deal with.
Both is separated by a //.
This is similar to the OCM notation <repo>//<component>:<version>
mandelsoft
force-pushed
the
mandelsoft/fix
branch
from
04e1992 to
4b9bcd5
Compare
What this PR does / why we need it:
Fix the tar input handling for the
ocm addcommands.So far in uses two nested directory traverses, which leads to the packaging of file multiple times.
This has been extracted from the #70
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: