Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

use docker/reference.Store to manage tags/digest by app image ID #782

Merged
merged 1 commit into from Jan 7, 2020

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Dec 10, 2019

- What I did
Used docker/reference to implement imageStore

- How I did it
only store images by ID, use docker/reference to track tags/digest

- How to verify it
e2e test suite cover most usages

- Description for the changelog
re-implemented bundle store

- A picture of a cute animal (not mandatory)
image

@ndeloof ndeloof force-pushed the refstore branch 5 times, most recently from 64651c3 to dc3e08d Compare December 12, 2019 16:04
@ndeloof ndeloof marked this pull request as ready for review December 12, 2019 16:34
@codecov
Copy link

codecov bot commented Dec 13, 2019

Codecov Report

Merging #782 into master will decrease coverage by 0.3%.
The diff coverage is 71.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
- Coverage   70.43%   70.13%   -0.31%     
==========================================
  Files          67       67              
  Lines        3734     3676      -58     
==========================================
- Hits         2630     2578      -52     
+ Misses        756      753       -3     
+ Partials      348      345       -3
Impacted Files Coverage Δ
internal/cnab/cnab.go 32.3% <0%> (ø) ⬆️
internal/store/digest.go 85.18% <100%> (+1.18%) ⬆️
internal/packager/bundle.go 64% <100%> (ø) ⬆️
internal/commands/image/tag.go 84.21% <100%> (ø) ⬆️
internal/commands/push.go 34.07% <66.66%> (-0.02%) ⬇️
internal/store/image.go 72.32% <70.21%> (-4.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df1c57e...e4c01b5. Read the comment docs.

@aiordache
Copy link
Contributor

LGTM

}
digest := digest.NewDigestFromEncoded(digest.SHA256, s)
return ID{digest}, nil
return ID(digest), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can juste return here the result of fromID ?

func FromString(s string) (ID, error) {
    if ok := identifierRegexp.MatchString(s); !ok {
        return "", fmt.Errorf("could not parse %q as a valid reference", s)
    }
    return fromID(s), nil

if err := os.RemoveAll(path); err != nil {
return nil
defer f.Close()
ids, err := f.Readdirnames(-1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this return directories like . or .. ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only actual directories

}
_, err = b.store.Delete(named)
references := b.store.References(id)
if len(references) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if len(references) == 0, we discard the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just mean we don't have tags for this, only a Digest

func build(t *testing.T, cmd icmd.Cmd, dockerCli dockerCliCommand, ref, path string) {
cmd.Command = dockerCli.Command("app", "build", "-t", ref, path)
func build(t *testing.T, cmd icmd.Cmd, dockerCli dockerCliCommand, ref, path string) string {
iidfile := fs.NewFile(t, "iid")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you use docker app build --quiet instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works as well, but I prefer iidfile :P

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Cool To Me !!!!

Copy link
Contributor

@aiordache aiordache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof merged commit 3064a5d into docker:master Jan 7, 2020
@ndeloof ndeloof deleted the refstore branch January 7, 2020 11:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants