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

neonvm-runner: Small cleanups #1185

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

Conversation

sharnoff
Copy link
Member

I was looking at trying to implement some changes to what neonvm-runner puts in the ISO 9660 filesystem, and quickly began hitting #1184, so decided to make some changes to fix that.

The commits are basically just what the subject messages say.


It's worth noting that these changes are kind of only surface-level; I think there's deeper structural changes that we should make so neonvm-runner can be easier to maintain and extend, but I'm not entirely sure what that should look like, and just breaking it up into separate files seemed less objectionable.

Best I can tell, this comment has gradually made less sense as new code
is placed between it and the actual createISO9660runtime() function
call, most recently with #864 / 63e1d72.

So, let's move it back to that function.
Looks like these were swapped out in c693804 / neondatabase/neonvm#5,
without stating where they came from.

I don't see a reason we need to keep our own copies of them, so let's
just switch back to the docker versions.
Basically, instead of 'go build path/to/main.go', we should instead
write 'go build path/to/*.go' -- otherwise, adding new files alongside
main.go will result in unexpected errors about items not being found.
main.go is huge, and this seems like a reasonable place to make a cut.
We probably should remove this anyways, but in the meantime, having it
all collected in one file keeps that out of the way, and should make it
easier to remove later.
Copy link

No changes to the coverage.

HTML Report

Click to open

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.

1 participant