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

feat: validate manifest before writing #159

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

letFunny
Copy link
Collaborator

@letFunny letFunny commented Oct 2, 2024

  • Have you signed the CLA?

Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left a comment below about a function name.

internal/manifest/manifest.go Show resolved Hide resolved
@letFunny letFunny added the Priority Look at me first label Oct 3, 2024
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Very nice, thanks. Only a trivial adding some notes to fastValidate.

@@ -301,3 +306,105 @@ func unixPerm(mode fs.FileMode) (perm uint32) {
}
return perm
}

// fastvalidate validates the data to be written into the manifest on disk.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please add:

// fastValidate validates the data to be written into the manifest.
// This is validating internal structures which are supposed to be correct unless there is
// a bug. As such, only assertions that can be done quickly are performed here, instead
// of it being a comprehensive validation of all the structures.

Copy link
Contributor

Choose a reason for hiding this comment

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

I went ahead and edited it as it's just an additional comment.

Name: "slice2",
}

var generateManifestTests = []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the new tests.

@niemeyer niemeyer merged commit 5974449 into canonical:main Oct 7, 2024
14 checks passed
@endersonmaia
Copy link

Sorry to make the question here.

What's the plan, add a an essential manifest slice to all packages like we have for copyright?

ex.:

package: libc6

essential:
  - libc6_copyright
  - libc6_manifest

slices:
(...)

  copyright:
    contents:
      /usr/share/doc/libc6/copyright:

  manifest:
    contents:
      /var/lib/chisel/**: {generate:  manifest}

@rebornplusplus
Copy link
Member

rebornplusplus commented Oct 15, 2024

It doesn't need to be in essential. If you install the libc6_manifest slice along with the other slices you want to install, you will get a valid chisel manifest at /var/lib/chisel/manifest.wall. It's a zstd-compressed, JSON file.

We actually do provide the slice as of this writing. It's base-files_chisel in the current releases.

@endersonmaia
Copy link

So, what I understood was:

base-files_chisel is a slice that when installed will feed the manifest with information from all other package slices installed together in the same chisel cut action.

And *_manifest is a "virtual" slice that's available for all packages, that when installed will feed the manifest with information from all other slices that are its dependencies on the same chisel cut action.

Am I right?

@rebornplusplus
Copy link
Member

base-files_chisel is a slice that when installed will feed the manifest with information from all other package slices installed together in the same chisel cut action.

Correct.

And *_manifest is a "virtual" slice that's available for all packages, that when installed will feed the manifest with information from all other slices that are its dependencies on the same chisel cut action.

Nope. *_manifest does not exist, not even virtually. You may create those, similar to base-files_chisel. But you don't really need multiple slices like base-files_chisel, unless you want to create multiple manifest file with (almost, if not) the same contents.

@letFunny letFunny deleted the validate-manifest-on-write branch October 17, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority Look at me first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants