devpod icon indicating copy to clipboard operation
devpod copied to clipboard

Fix feature install order

Open aacebedo opened this issue 1 year ago • 4 comments

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 avatar Apr 27 '24 16:04 aacebedo

@pascalbreuninger can you have a look please?

aacebedo avatar Apr 27 '24 21:04 aacebedo

@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?

pascalbreuninger avatar Apr 28 '24 08:04 pascalbreuninger

@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.

aacebedo avatar Apr 29 '24 07:04 aacebedo

@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

pascalbreuninger avatar Apr 29 '24 15:04 pascalbreuninger