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

Fix feature install order #1033

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aacebedo
Copy link
Contributor

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 aacebedo changed the title fix: fix feature install order Fix feature install order Apr 27, 2024
@aacebedo
Copy link
Contributor Author

@pascalbreuninger can you have a look please?

@pascalbreuninger
Copy link
Member

pascalbreuninger commented Apr 28, 2024

@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 aacebedo force-pushed the aacebedo/fix_feature_install_order branch from 08af7e3 to 7eedfad Compare April 29, 2024 07:14
@aacebedo
Copy link
Contributor Author

@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
Copy link
Member

pascalbreuninger commented Apr 29, 2024

@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

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.

2 participants