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

Rename relocated.bundle to AppImage #773

Merged
merged 1 commit into from
Dec 6, 2019
Merged

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Nov 28, 2019

- What I did
Renamed relocated.bundle to AppImage
bundleStore to ImageStore
bndl variables to img

- How I did it
asked my IDE to do the job for me

- How to verify it
You don't read code using a bundle that is more than just a cnab bundle.

- Description for the changelog
Introduced AppImage type and ImageStore

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

@ndeloof ndeloof marked this pull request as ready for review November 29, 2019 09:48
}

func getRelocatedBundle() (*relocated.Bundle, error) {
func getRelocatedBundle() (*image.AppImage, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: What about image.App ? image.AppImage sounds like repetition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better rename package then. "App" is highly confusing with "running app".

"github.com/docker/distribution/reference"
multierror "github.com/hashicorp/go-multierror"
digest "github.com/opencontainers/go-digest"
"github.com/pkg/errors"
)

//
type BundleStore interface {
type ImageStore interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: Is it an ImageStore or an AppImageStore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also wondering. wanted to avoid too long names (hey, this is Golang :P) but AppImageStore would better describe

@codecov
Copy link

codecov bot commented Dec 2, 2019

Codecov Report

Merging #773 into master will decrease coverage by 0.08%.
The diff coverage is 87.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #773      +/-   ##
==========================================
- Coverage    70.4%   70.32%   -0.09%     
==========================================
  Files          67       67              
  Lines        3740     3740              
==========================================
- Hits         2633     2630       -3     
- Misses        760      762       +2     
- Partials      347      348       +1
Impacted Files Coverage Δ
internal/store/installation.go 78.12% <100%> (ø) ⬆️
internal/commands/run.go 62.36% <100%> (ø) ⬆️
internal/packager/bundle.go 64% <100%> (ø) ⬆️
internal/commands/image/list.go 81.53% <100%> (ø) ⬆️
internal/commands/pull.go 64.28% <100%> (ø) ⬆️
internal/commands/build/build.go 62.23% <100%> (ø) ⬆️
internal/store/app.go 73.33% <100%> (ø) ⬆️
internal/commands/image/inspect.go 66.66% <100%> (ø) ⬆️
internal/store/digest.go 84% <100%> (ø) ⬆️
internal/commands/image/tag.go 84.21% <100%> (ø) ⬆️
... and 9 more

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 6df0ec9...9873b2f. Read the comment docs.

also rename bundleStore to imagestore
and variables pointing to relocated.bundles to `img`

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
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.

LGTM

Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

LGTM

@ndeloof ndeloof merged commit 012748d into docker:master Dec 6, 2019
@ndeloof ndeloof deleted the imageStore branch December 6, 2019 09:07
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.

4 participants