Add attachments (extra files) into the package #392
Conversation
mikeparker
force-pushed
the
bundle-external-files
branch
2 times, most recently
from
b2e0356
to
6dc5bd3
Compare
internal/packager/fork.go
Outdated
| // Check we aren't doing ./../../../ etc in the path | ||
| fullFilepath := filepath.Join(appDir, convertedFilepath) | ||
| _, err := filepath.Rel(appDir, fullFilepath) | ||
| if err != nil { |
There was a problem hiding this comment.
This function is basically the same in fork.go and registry.go, there is a minor difference whereby fork alters the metadata file. Should I extract the duplication?
There was a problem hiding this comment.
Yep I guess you can try yes 😄
There was a problem hiding this comment.
Ok, i've pushed an attempt at this. There were 2 options: Inject the metadata editing code into the extracted function via a func parameter (ugly), or extract all the files onto disk once then update the metadata file after extraction. I went for the latter as it seemed cleaner, however this is slightly slower. Thoughts?
internal/inspect/inspect.go
Outdated
| externalFiles := app.ExternalFilePaths() | ||
| printSection(out, len(externalFiles), func(w io.Writer) { | ||
| for _, name := range externalFiles { | ||
| fmt.Fprintln(w, name) // and info.Size() ? |
There was a problem hiding this comment.
Remove the comment?
|
I'd be interested in ideas about what to call the files to store (not the docker-compose.yml etc)
I can't think of the right name. |
mikeparker
force-pushed
the
bundle-external-files
branch
from
bd175f6
to
e9ecb6b
Compare
mikeparker
force-pushed
the
bundle-external-files
branch
from
e9ecb6b
to
ef5c5d2
Compare
mikeparker
force-pushed
the
bundle-external-files
branch
from
cdb41ca
to
cb9330d
Compare
|
I added the filesize output to inspect, and renamed External Files -> Attachments |
mikeparker
changed the title
mikeparker
changed the title
|
Codecov Report
@@ Coverage Diff @@
## master #392 +/- ##
==========================================
- Coverage 61.29% 58.04% -3.26%
==========================================
Files 59 59
Lines 3462 3246 -216
==========================================
- Hits 2122 1884 -238
- Misses 1082 1095 +13
- Partials 258 267 +9
Continue to review full report at Codecov.
|
…t has the external file in Signed-off-by: Mike Parker <michael.parker@docker.com>
…ernal files from a .dockerapp. Logic will exclude the core 3 files from the root dir. Test with files in nested directories being reported correctly Test for sorting files alphabetically Signed-off-by: Mike Parker <michael.parker@docker.com>
…the inspect output; add external files to the loader Signed-off-by: Mike Parker <michael.parker@docker.com>
…; delete comments Signed-off-by: Mike Parker <michael.parker@docker.com>
Signed-off-by: Mike Parker <michael.parker@docker.com>
Signed-off-by: Mike Parker <michael.parker@docker.com>
…in the payload Signed-off-by: Mike Parker <michael.parker@docker.com>
…pect Signed-off-by: Mike Parker <michael.parker@docker.com>
Signed-off-by: Mike Parker <michael.parker@docker.com>
Signed-off-by: Mike Parker <michael.parker@docker.com>
…payload and in the app object in memory. Translate only when working with the filesystem. Fix other nits. Signed-off-by: Mike Parker <michael.parker@docker.com>
Signed-off-by: Mike Parker <michael.parker@docker.com>
…ebase. Signed-off-by: Mike Parker <michael.parker@docker.com>
Signed-off-by: Mike Parker <michael.parker@docker.com>
…ery config.cfg to be different Signed-off-by: Mike Parker <michael.parker@docker.com>
Signed-off-by: Mike Parker <michael.parker@docker.com>
mikeparker
force-pushed
the
bundle-external-files
branch
from
081a9fe
to
395eba4
Compare
| appPath := filepath.Join(outputDir, internal.DirNameFromAppName(name)) | ||
| if err := os.MkdirAll(appPath, 0755); err != nil { | ||
| appDir := filepath.Join(outputDir, internal.DirNameFromAppName(name)) | ||
| if err := os.MkdirAll(appDir, 0755); err != nil { |
There was a problem hiding this comment.
I think this MkdirAll should be changed to a Mkdir. I know it was like this before but it means that you can run docker-app fork -p /tmp/path/does/not/exist ccrone/hello-world.dockerapp:0.1.0 and it will create all the directories from /tmp/ onwards.
I would be OK to err out if the parent directory doesn't exist.
@silvin-lubecki WDYT?
There was a problem hiding this comment.
I think we should do that in a follow-up, so we can focus on it separately.
Signed-off-by: Mike Parker <michael.parker@docker.com>
…ious refactor) Signed-off-by: Mike Parker <michael.parker@docker.com>
|
I''ve decided to not do anything about the The reasons are:
|
types/types_test.go
Outdated
| @@ -163,8 +163,9 @@ func TestWithAttachmentsAndNestedDirectories(t *testing.T) { | |||
| app, err := NewAppFromDefaultFiles(dir.Path()) | |||
| assert.NilError(t, err) | |||
| assert.Assert(t, is.Len(app.Attachments(), 2)) | |||
| assert.Assert(t, is.Equal(app.Attachments()[0].FilePath(), "config.cfg")) | |||
| assert.Assert(t, is.Equal(app.Attachments()[1].FilePath(), "nesteddirectory/nestedconfig.cfg")) | |||
| assert.Assert(t, is.Equal(app.Attachments()[0].Path(), "config.cfg")) | |||
There was a problem hiding this comment.
nit: There's a helper to do this:
assert.Equal(t, x, y, msg)
|
Agree that it doesn't make sense to change the units library. There's a broader conversation to be had around how we handle vendored libraries (and versions of the libraries) across the Docker codebase. |
Signed-off-by: Mike Parker <michael.parker@docker.com>
|
Documentation:
|
- What I did
Added ability to attach files to a docker-app package (e.g. config files) including:
Misc
../../../bin/somethingetc, those files will be ignoredNOTE: The following have not been done (yet)
- Description for the changelog
Added ability to attach files to a docker-app package (e.g. config files)
- A picture of a cute animal (not mandatory but encouraged)