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 dotEtch generation and streaming methods #277

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

Conversation

aethernet
Copy link
Contributor

required for preloading

change-type: minor

@aethernet
Copy link
Contributor Author

This is the same as #257 but with the fix (I deleted the branch for the other PR by mistake)

Copy link
Contributor

@dfunckt dfunckt left a comment

Choose a reason for hiding this comment

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

This is great stuff. Some general thoughts:

  1. I think a versioning scheme is worth devising early, for both when requesting etchfiles and when applying them. A simple major/minor should be fine, and clients/servers must reject majors larger than they know about and should make reasonable effort to work with older ones. This is only pertinent to this PR because you will likely need to inject the version somewhere in the stream.
  2. Can you please explain (once more :) why the supervisor needs special handling?
  3. Does the .etch format support (or how hard can it be made to support) the base image and injectables being optional? So you can have injectables without a base image, or a base image without injectables (far less useful). Similarly, can the base image be a manifest instead that enables everything etcher currently supports? Eg. from URL.
  4. console.log is fine for the first versions, but we should ultimately use a logger function/object/stream that is passed as an argument to the entrypoint so we can integrate logging with whatever clients use.

lib/dotetch/registry.ts Outdated Show resolved Hide resolved
lib/dotetch/registry.ts Outdated Show resolved Hide resolved
@aethernet
Copy link
Contributor Author

aethernet commented Feb 3, 2023

All very valid points :

  1. Absolutely, I'll add back the manifest and add put the version

  2. I know we'd prefer not to have anything related to balena use case in here. Until we support multiapp and hostapps becomes one app beside users app (and even there I'm not sure how we'll start supervisor), we need to tag supervisor specifically in repositories.json otherwise it couldn't be started by the os. I could find a way to make this more generic but I'd prefer to not delay the feature further.

  3. Yes, once the stream is open, you can push anything (or nothing) in it before you close it. But, as it's a tar, the order of operations is important as the order files are delivered matter. So you can omit a base image but you cannot send it after the preloading assets. Same applies for preloading assets. In theory nothing prevent to just ship a manifest.

  4. Noted. I didn't thought much of it as in image-maker they're captured and properly treated, but you're totally right that it need to be more flexible for other users

@aethernet
Copy link
Contributor Author

aethernet commented Feb 3, 2023

I've added the manifest with a version 1.0 (1), it will be trivial to extend it to handle "url" for any asset in the stream (3)

@aethernet aethernet force-pushed the aethernet/dotetchpreloading branch 3 times, most recently from 66fc0cf to e52f1f8 Compare March 8, 2023 15:02
@jellyfish-bot
Copy link

[aethernet] This has attached https://jel.ly.fish/62ec39b3-f868-46cd-ad7c-3d375b83f8ac

@aethernet aethernet force-pushed the aethernet/dotetchpreloading branch 2 times, most recently from c257dbd to 380e7c2 Compare May 22, 2023 12:11
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.

3 participants