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

[WIP] Add Update #109

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[WIP] Add Update #109

wants to merge 2 commits into from

Conversation

afflom
Copy link

@afflom afflom commented Oct 17, 2022

No description provided.

@afflom afflom requested a review from jpower432 October 17, 2022 06:37
@afflom afflom changed the title [WIP] {feat) Add Update and Delete [WIP] Add Update and Delete Oct 17, 2022
@afflom afflom changed the title [WIP] Add Update and Delete [WIP] Add Update Oct 17, 2022
collectionManifestAnnotations := map[string]string{}
var descs []ocispec.Descriptor

if config.Collection.SchemaAddress != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this section any different from the schema validation in the Build method?

func (d DefaultManager) Update(ctx context.Context, space workspace.Workspace, src string, dest string, add bool, remove bool, client registryclient.Client) (string, error) {

// Pull the dataset config from the target collection
_, manBytes, err := client.GetManifest(ctx, src)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this type of action could benefit from a dry run or if it could just construct the workspace and pass it to the top-level Build method. Just so the user can validate the workspace before any references are overwritten.

Copy link
Contributor

@jpower432 jpower432 left a comment

Choose a reason for hiding this comment

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

A few design comments/questions.

}

if remove {
pruneDescs, err := d.pullCollection(ctx, src, memory.New(), client)
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on what files are being deleted, this action could run the system out of memory since this is storing all of the files as byte slices.

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.

2 participants