devpod
devpod copied to clipboard
loft-sh
fix(#163): handle objects in the devcontainer lifecyle hooks
Following on from #380, this is a second attempt at solving #163 thanks to @pascalbreuninger's previous review.
Fixes #163
This converts objects to an array of strings using a parallel Bash command. This does assume that bash is present on the devcontainers, which is probably a safe bet but I guess we could switch to sh if necessary.
@mrsimonemms Thanks for giving this another shot! I don't feel good about bending the existing implementation to accomodate for the object based commands. Imo we should rather refactor the way we currently parse the commands so we're spec compliant.
If you want to go ahead and do that, we'll do our best to support you. If not we'll put it into our roadmap and will work on it rather sooner than later
@pascalbreuninger yeah, I'm sort of with you on that. This is probably "good enough" for testing/known repos, but not for GA.
I'm happy to have a bash at it properly - can you give me a clue as to whereabouts in the code base I should be looking for the lifecycle commands and the implementation?
@mrsimonemms you can find that at https://github.com/loft-sh/devpod/blob/main/pkg/devcontainer/setup/setup.go#L232, I guess we can still execute these sequentially in the beginning if thats easier to implement and then eventually do the proper execute at the same time implementation.
@FabianKramm thanks. I was looking in the right place then 🎉
As an aside, there's no initializeCommand which is documented - I'll do a second PR to include that once I've got this accepted.
The further thought occurs that maybe a future refactoring could be done to use the JSON schema to ensure validity with the standard. I've got a couple of ideas on how this could be done, but this is probably something that should be specified fully by a maintainer first as it's likely to be a big old change.
Hey @pascalbreuninger, I've had another bash at this based on your feedback. Let me know what you think.
I've created the new LifecycleHook type and I've also change the InitializeCommand key to use it, as per the DevContainer spec. I've tested it against the Docker provider and my Hetzner provider and both seem to work fine.
@mrsimonemms thanks for the contribution and patience! LGTM as well
@pascalbreuninger I'd gone far enough into this one that I was determined to fix it 🤣