Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(*): add publishing from an archive #700

Merged
merged 14 commits into from
Oct 14, 2019
Merged

feat(*): add publishing from an archive #700

merged 14 commits into from
Oct 14, 2019

Conversation

vdice
Copy link
Member

@vdice vdice commented Oct 4, 2019 •
edited
Loading

What does this change

  • add --archive and --tag flags to porter publish
  • defer to tag when publishing bundle (from archive or not)
  • wire up archive importer/loader and then gen new bundle with updated images
  • new/published bundle will have new images (invocation and other) with updated registry/orgs as derived from --tag value

TODO:

  • address inline/code TODOs
  • add test for general publishing from archive scenario (needs to be integration)? units added and integration test
  • Currently, we don't attempt to change/update the invocation image on publishing from an archive; just the bundle tag. Do so in this PR? From --tag or add new --registry flag, etc.?

What issue does it fix

Towards #545

Notes for the reviewer

Checklist

  • Unit Tests
  • Documentation
    • Documentation Not Impacted

carolynvs
carolynvs reviewed Oct 4, 2019
View reviewed changes
cmd/porter/bundle.go Outdated Show resolved Hide resolved
cmd/porter/bundle.go Outdated Show resolved Hide resolved
pkg/porter/publish.go Outdated Show resolved Hide resolved
pkg/porter/publish.go Outdated Show resolved Hide resolved
@vdice vdice force-pushed the feat/publish-from-archive branch 2 times, most recently from b42474a to fbab719 Compare October 8, 2019 17:20
@vdice vdice changed the title WIP feat(*): add publishing from an archive feat(*): add publishing from an archive Oct 8, 2019
@vdice vdice marked this pull request as ready for review October 8, 2019 17:23
@vdice vdice requested a review from jeremyrickard as a code owner October 8, 2019 17:23
@vdice vdice force-pushed the feat/publish-from-archive branch from 964c8fe to c77fa83 Compare October 8, 2019 22:16
@vdice vdice requested a review from radu-matei October 8, 2019 22:36
@vdice vdice force-pushed the feat/publish-from-archive branch from c77fa83 to 79068bd Compare October 8, 2019 23:25
carolynvs
carolynvs approved these changes Oct 9, 2019
View reviewed changes
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/porter/publish.go Outdated
return err
}

digest, err := p.Registry.Copy(img, newImg)
Copy link
Member

Choose a reason for hiding this comment

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

So when we publish an archived bundle, the contents of the bundle don't really matter (the referenced images). We use the tags from where the images were originally published, and rely on being able to copy from there to the new location. We don't copy from the archive itself. Am I understanding that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good clarification -- indeed, this is the current approach.

As I understand it, the functionality for importing images from an OCI Image layout (i.e., images in the /artifacts dir of the archive) into the Docker daemon is not yet available (see cnabio/cnab-go#136)

Should I add a note/comment in-line mentioning this? Would it be prudent to add a ticket in our backlog to revisit this area, If/when the aforementioned functionality is available? (To import images straight from the archive into the local daemon and then re-tag/push.)

Copy link
Contributor

Choose a reason for hiding this comment

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

So how does this work with the offline scenario where the bundle is on an air-gapped network?

I think we need to use the Pivotal relocation library here and copy FROM the archive up to a new registry. Loading things into the local docker daemon is nice, but for any case that involves a registry, it is kind of irrelevant isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm good point. I'll work on the layout relocation approach directly from the archive and update.

Copy link
Member Author

@vdice vdice Oct 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ok @carolynvs & @jeremyrickard , updated logic to use the relocation lib to parse extracted OCI Layout and push renamed imgs directly from the archived images (same digests). Did find one issue which I'll raise in the image-relocation repo, but I don't think it is a blocker (and I believe the fix would be in this dependency, anyhow). edit: added comment to pre-existing issue in pivotal/image-relocation

@vdice vdice force-pushed the feat/publish-from-archive branch 4 times, most recently from beaef16 to 94a8186 Compare October 10, 2019 20:03
jeremyrickard
jeremyrickard approved these changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@jeremyrickard jeremyrickard left a comment

Choose a reason for hiding this comment

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

LGTM, let's merge this and play around with it for a while.

@vdice do we have issues to update cnab-go, I see you're refing a branch of your cnab-go fork?

vdice added 14 commits October 14, 2019 09:42
@vdice
WIP feat(*): add publishing from an archive
c627331 
- add --archive and --tag flags to porter publish
- defer to tag when publishing bundle (from archive or not)
- wire up archive importer/loader and publish bundle once loaded
@vdice
mention required --tag when porter publish --archive is invoked
99bc6ba
@vdice
feat(publish.go): add image renaming/updating to publish --archive
6bd00c0
@vdice
update cnab-go dep with branch off latest v0.6.0-beta.1 release
366f3da
@vdice
rebase with master
3b48d1e
@vdice
debug to p.Err
b934157
@vdice
add extensive notes/comments to publishFromArchive to mention current…
0b9cc9e 
… approach
@vdice
add porter archive/publish integration test
8b16438
@vdice
ref(publish.go): update/push images directly from extracted OCI Layout
04c61f4
@vdice
update archive integration test to pre-publish before archive
64c831a
@vdice
remove unused Copy method from Registry interface
8075d22
@vdice
update publishFromArchive notes
c09ff0c
@vdice
update deps; fix archive_test.go
31eaa20
@vdice
update cnab-go dep to v0.7.1-beta.1
8a3d1bb
@vdice vdice force-pushed the feat/publish-from-archive branch from 94a8186 to 8a3d1bb Compare October 14, 2019 15:47
@vdice vdice merged commit d9d0754 into getporter:master Oct 14, 2019
@vdice vdice deleted the feat/publish-from-archive branch October 14, 2019 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyrickard jeremyrickard jeremyrickard approved these changes

@carolynvs carolynvs carolynvs approved these changes

@radu-matei radu-matei Awaiting requested review from radu-matei

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vdice @carolynvs @jeremyrickard