sdk: Trim away lots of CL stuff #1195
Conversation
The code in `sdk` closely matches what we do with coreos-assembler in the concept of a "workdir". Trim out the CL-specific stuff like `REPO_ROOT` environment variables, etc. This is prep for reworking it to detect things like `builds/` from coreos-assembler. Now, the "Fedora Cloud Base using plume" code *might* have used that but I doubt it. It basically boils down to "pwd" so let's just assume that in that code path for now.
|
/cc @dustymabe who was leading the effort around Fedora Cloud Base using mantle. I don't think this will break it but just FYI. |
| return os.Setenv("SSH_AUTH_SOCK", chrootPath) | ||
| } | ||
|
|
||
| // MountGnupg bind mounts $GNUPGHOME or ~/.gnupg and the agent socket |
There was a problem hiding this comment.
Note that since this code was created in 2015 we've basically made this concept of "run some code unprivileged in a container sharing my home directory" a first class concept.
| func getFedoraImageFile(client *http.Client, spec *channelSpec, src *storage.Bucket, fileName string) (string, error) { | ||
| cacheDir := filepath.Join(sdk.RepoCache(), "images", specChannel, specVersion) | ||
| rawxzPath := filepath.Join(cacheDir, fileName) | ||
| imagePath := strings.TrimSuffix(rawxzPath, ".xz") | ||
| imagePath := strings.TrimSuffix(fileName, ".xz") |
There was a problem hiding this comment.
I think this might be problematic. It looks like sdk.UpdateFile() wants a full local file path where the user wants the file to be downloaded to. I believe you changed this because sdk.RepoCache() is changing. Maybe we create our own temporary directory here and use that?
The only drawback I see of that is that I think the sdk download logic would resume downloads if one had failed for whatever reason and if we don't use the same directory each time we'll be throwing away that logic.
There was a problem hiding this comment.
Let me ask a higher level question: Is this code actually used today or is it still just a PoC?
Second question related to that - is that code setting REPO_PATH, or was it manually doing mkdir?
Actually, where is the Fedora infra code that runs this located?
There was a problem hiding this comment.
Backing up what I'd like to get to is where this sdk parses coreos-assembler schema and that's it.
So one thing non-CoreOS systems could do is "fake it" and create a builds/version/x86_64/meta.json that has a vmdk image etc. That shouldn't be too hard to do.
There was a problem hiding this comment.
The code is not currently used today. There is an example of how to use it in the README. There is also the joystick app that is what sayan was working on to actually use it before he left.
This code seems to be building up a URL to a disk image using all of the information (composeID, image type, arch, filename, etc) and then downloading it using sdk.UpdateFile(). Previously we were calling sdk.UpdateFile() with rawxzPath with is a path we had constructed using filepath.Join(sdk.RepoCache(), "images", specChannel, specVersion). I don't know how important that was or not. We can probably just choose our own temporary directory and use that.
There was a problem hiding this comment.
This path is just an optimization to reuse a local image if it exists. As far as I can tell joystick isn't doing that.
So I think this is fine to ship.
|
Makes sense to me! Will let @dustymabe have a final look. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, dustymabe, jlebon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kola: Fix syncOptions
The code in
sdkclosely matches what we do with coreos-assemblerin the concept of a "workdir". Trim out the CL-specific stuff
like
REPO_ROOTenvironment variables, etc.This is prep for reworking it to detect things like
builds/from coreos-assembler.
Now, the "Fedora Cloud Base using plume" code might have
used that but I doubt it. It basically boils down to "pwd" so
let's just assume that in that code path for now.