search

loft-sh / devpod

Codespaces but open-source, client-only and unopinionated: Works with any IDE and lets you use any cloud, kubernetes or just localhost docker.
https://devpod.sh
Mozilla Public License 2.0
8.43k stars 307 forks source link

Fix feature install order #1033

Open aacebedo opened 2 months ago

aacebedo commented 2 months ago

When I tried to install features by specifying the order in their manifest I noticed it was not working (with local feature). I saw that the ID with which the ID was compared was not the ID of the manifest but the name given in the devcontainer.json.

This PR fixes it.

aacebedo commented 2 months ago

@pascalbreuninger can you have a look please?

pascalbreuninger commented 2 months ago

@aacebedo I'm a bit concerned about using Config.ID in a function shared by both the automatic detection and manual feature order. It might make sense to change computeFeatureOrder instead of computeAutomaticFeatureOrder

Could you add an example please?

aacebedo commented 2 months ago

@aacebedo I'm a bit concern about using Config.ID in a function shared by both the automatic detection and manual feature order. It might make sense to change computeFeatureOrder instead of computeAutomaticFeatureOrder

Could you add an example please?

Thanks for the fast answer. I have pushed an example.

I cannot not change the automaticFeatureOrder at it is this function which deal with the field of the feature manifest.

Now, I also checked the container.dev spec (especially this section https://containers.dev/implementors/features/#referencing-a-feature), and it seems that the ID they mention is the one used to reference the feature in the devcontainer.json file. It is very weird to me because, to me, feature manifest shall not depend on the location pr the way dependent features are gathered.

What do you think? I can close the PR if you think my initial interpretation is wrong.

pascalbreuninger commented 2 months ago

@aacebedo thanks for the example! Not sure I follow completely, I'll need a bit of time to take a look at the spec again. In the meantime would using overrideFeatureInstallOrder (search here) work for your use case? Just to get you unblocked until we merge a potential fix