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

Saving bundles as an intermediate ocitar #612

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

ashpect
Copy link
Contributor

@ashpect ashpect commented Nov 24, 2023

Implements this proposal.
Implemented except writing the tests, waiting for initial review, after which e2e tests can be written.

Screenshot 2024-05-23 at 10 14 29 PM

@ashpect ashpect temporarily deployed to TanzuNet Registry Dev e2e November 24, 2023 10:34 — with GitHub Actions Inactive
@ashpect ashpect temporarily deployed to TanzuNet Registry Dev e2e November 28, 2023 23:29 — with GitHub Actions Inactive
@ashpect ashpect had a problem deploying to TanzuNet Registry Dev e2e November 29, 2023 02:17 — with GitHub Actions Failure
@ashpect ashpect had a problem deploying to TanzuNet Registry Dev e2e December 8, 2023 03:42 — with GitHub Actions Failure
@ashpect ashpect had a problem deploying to TanzuNet Registry Dev e2e March 3, 2024 15:19 — with GitHub Actions Failure
@ashpect ashpect had a problem deploying to TanzuNet Registry Dev e2e March 3, 2024 15:23 — with GitHub Actions Failure
@ashpect ashpect had a problem deploying to TanzuNet Registry Dev e2e March 3, 2024 15:23 — with GitHub Actions Failure
@ashpect ashpect had a problem deploying to TanzuNet Registry Dev e2e March 20, 2024 12:25 — with GitHub Actions Failure
@ashpect ashpect had a problem deploying to TanzuNet Registry Dev e2e May 22, 2024 07:08 — with GitHub Actions Failure
@ashpect ashpect temporarily deployed to TanzuNet Registry Dev e2e May 22, 2024 07:09 — with GitHub Actions Inactive
@ashpect ashpect had a problem deploying to TanzuNet Registry Dev e2e May 22, 2024 07:50 — with GitHub Actions Failure
@ashpect ashpect had a problem deploying to TanzuNet Registry Dev e2e May 22, 2024 07:54 — with GitHub Actions Failure
Signed-off-by: ashpect <[email protected]>
Signed-off-by: ashpect <[email protected]>
Signed-off-by: ashpect <[email protected]>
Signed-off-by: ashpect <[email protected]>
Signed-off-by: ashpect <[email protected]>
Signed-off-by: ashpect <[email protected]>
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, made some comments in the code.
On a more generic note:

  • We need to add some tests to this new feature
  • Can we use ggcr to read/write the tarballs for us, since this is a regular OCI tar we should be able to use something like https://github.com/google/go-containerregistry/tree/main/pkg/v1/tarball to do this work.
  • In terms of form, I think I would prefer us to provide variables like ociTarPath to the Push functions instead of providing them to the constructors since that has been the pattern we have been using. The idea here is that when you call the function you choose the Repository/location where you want to store the result. Another option that might make more sense here is instead of calling NewContents().Push maybe we should have a new function function called NewContents().PushToTar() since we do not need the majority of the code in Push because we are not building tags or writing them

Comment on lines +87 to +90
processedImages, err = c.tarImageSet.Import(tempDir, importRepo, c.registry, true)
if err != nil {
return nil, fmt.Errorf("Importing OCI tar: %s", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Since this is similar to the else case, maybe we can move this outside the if statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The else case is calling the import function with false as a parameter while here its true which in turns calls the readoci tar different to read tar depending on the bool in the import function.
Screenshot 2024-05-25 at 1 45 45 PM

if err != nil {
return nil, err
}
err = image.ExtractOciTarGz(c.OciFlags.OcitoReg, tempDir)
Copy link
Member

Choose a reason for hiding this comment

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

Let us make sure we clean up after ourselves

Suggested change
err = image.ExtractOciTarGz(c.OciFlags.OcitoReg, tempDir)
defer os.RemoveAll(tempDir)
err = image.ExtractOciTarGz(c.OciFlags.OcitoReg, tempDir)

if processedImage.ImageIndex != nil {
continue
}
// This is added to not read the lockfile and change the ref for oci-flag. Will be removed once we add an inflate option to copy the refs.
Copy link
Member

Choose a reason for hiding this comment

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

Is this commented out because if we copy from oci-tar to the repository, we do not have all the images? If that is the case, we should change the comment to highlight something like:

// When copying images from an oci-tar to a repository, imgpkg will not try to access the origin repositories and will only copy the OCI Image to the registry, similar to the behavior we currently have on the `imgpkg push` command.

Comment on lines 163 to 167
err = os.RemoveAll(tempDir)
if err != nil {
fmt.Println("Error cleaning up temporary directory:", err)
}

Copy link
Member

Choose a reason for hiding this comment

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

Added a prior comment that I think it would be better to use than this part of the code. Because if anything fails in the meanwhile we will leave the folder back

Suggested change
err = os.RemoveAll(tempDir)
if err != nil {
fmt.Println("Error cleaning up temporary directory:", err)
}

@@ -0,0 +1,23 @@
// Copyright 2023 The Carvel Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2023 The Carvel Authors.
// Copyright 2024 The Carvel Authors.


// CreateOciTarFileAndDeleteFolder creates a oci tar file from the source folder and deletes the source folder. Used while pushing the oci-tar
func CreateOciTarFileAndDeleteFolder(source, target string) error {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@@ -180,3 +181,119 @@ func (i *TarImage) isExcluded(relPath string) bool {
}
return false
}

// CreateOciTarFileAndDeleteFolder creates a oci tar file from the source folder and deletes the source folder. Used while pushing the oci-tar
Copy link
Member

Choose a reason for hiding this comment

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

Why are we deleting the source folder?
If I do imgpkg push -b repo/app1-config -f config/ --to-oci-tar /path/to/file.tar is my config folder going to be deleted? We should not do that


// ExtractOciTarGz extracts the oci tar file to the extractDir
func ExtractOciTarGz(inputDir, extractDir string) error {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@@ -0,0 +1,156 @@
// Copyright 2023 The Carvel Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2023 The Carvel Authors.
// Copyright 2024 The Carvel Authors.

Signed-off-by: ashpect <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants