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.

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

Add attach command #433

Merged
merged 63 commits into from
Jul 8, 2022
Merged

Add attach command #433

merged 63 commits into from
Jul 8, 2022

Conversation

qweeah
Copy link
Contributor

@qweeah qweeah commented Jul 4, 2022

Introducing a new simplified experience to add an artifact to an existed manifest:

  • a new command attach to add an artifact to a existed manifest
  • refactor packing code
  • refactor status tracking

Related to #424

@qweeah qweeah marked this pull request as draft July 4, 2022 09:13
@qweeah qweeah changed the title Add attach command [WIP] Add attach command Jul 4, 2022
@qweeah qweeah changed the title [WIP] Add attach command Add attach command Jul 7, 2022
@qweeah qweeah marked this pull request as ready for review July 7, 2022 03:29
internal/content/file.go Outdated Show resolved Hide resolved
cmd/oras/internal/input/file_unix.go Outdated Show resolved Hide resolved
cmd/oras/internal/input/file_unix.go Outdated Show resolved Hide resolved
cmd/oras/internal/input/file_windows.go Outdated Show resolved Hide resolved
dependabot bot and others added 24 commits July 7, 2022 21:40
Bumps [github.com/docker/cli](https://github.com/docker/cli) from 20.10.16+incompatible to 20.10.17+incompatible.
- [Release notes](https://github.com/docker/cli/releases)
- [Commits](docker/cli@v20.10.16...v20.10.17)

---
updated-dependencies:
- dependency-name: github.com/docker/cli
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <qweeah@gmail.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <qweeah@gmail.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <qweeah@gmail.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Juncheng Zhu <junczhu@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
}
}

// LoadFiles loads file reference to a file store and and returns the
Copy link
Member

Choose a reason for hiding this comment

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

nit: file references?

internal/content/file.go Outdated Show resolved Hide resolved

// Pusher option struct.
type Pusher struct {
ManifestExport string
Copy link
Member

Choose a reason for hiding this comment

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

maybe ManifestExportPath?


// ApplyFlags applies flags to a command flag set.
func (opts *Pusher) ApplyFlags(fs *pflag.FlagSet) {
fs.StringVarP(&opts.ManifestExport, "export-manifest", "", "", "export the pushed manifest")
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a true/false flag. I think we need to call out this is the file path of the exported manifest.

}

// ExportManifest saves the pushed manifest to a local file.
func (opts *Pusher) ExportManifest(ctx context.Context, pushed ocispec.Descriptor, pushedTo content.Fetcher) error {
Copy link
Member

Choose a reason for hiding this comment

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

Should we have some unit tests for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will create an issue to add unit test for pusher options

cmd/oras/internal/display/status.go Outdated Show resolved Hide resolved
cmd/oras/push.go Outdated Show resolved Hide resolved
}

// OCIToArtifact converts OCI descriptor to artifact descriptor.
func OCIToArtifact(desc ocispec.Descriptor) *artifactspec.Descriptor {
Copy link
Member

Choose a reason for hiding this comment

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

Can we just return a struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since & is banned for function return values, code will be cleaner if we returns a pointer

cmd/oras/attach.go Outdated Show resolved Hide resolved
cmd/oras/attach.go Outdated Show resolved Hide resolved
qweeah added 25 commits July 8, 2022 17:05
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@shizhMSFT shizhMSFT merged commit 8125941 into oras-project:main Jul 8, 2022
@qweeah qweeah deleted the attach branch July 11, 2022 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants