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

Feature Dependencies - installsAfter not respected #163

Closed
virtualstaticvoid opened this issue Sep 6, 2022 · 25 comments
Closed

Feature Dependencies - installsAfter not respected #163

virtualstaticvoid opened this issue Sep 6, 2022 · 25 comments
Assignees
Labels
bug Something isn't working verified
Milestone

Comments

@virtualstaticvoid
Copy link

virtualstaticvoid commented Sep 6, 2022

Hello,

I've written a couple of features for a project I'm working on and I've ran into situations where I'm not sure what the best approach to take is.

For example, I have a feature which involves installing a CLI tool using npm. Now, I'm not sure whether this feature should also install Node.js so that it is able to install the CLI tool, since I have no control over the base image, nor the included features in the devcontainer.json file.

The power of features, as I understand, is that they can be composed together; making each serve one purpose (installing the given feature) thus providing a simple way to handle all the tooling permutations for many different developers and/or teams whilst avoiding "golden container images"; and similar perhaps in concept to buildpacks.

So it occurred to me that since devcontainers/features already provide features for installing Node.js (and other language runtimes), it would be beneficial to make my feature dependant on the node feature. Then I wouldn't need to worry about which base image a developer is using, and the node feature would be automatically included before mine, so my feature would install reliably.

Additional scenarios include:

  • a Ruby gem which needs to build a native extension and thus requires build-essential to be installed.
  • a Python package, installed via pip, needing to ensure Python and pip are installed.
  • a Go CLI tool, which doesn't have a binary distribution, installed using go install, needing go to be installed.
  • a tflint ruleset for AWS, which requires Terraform and tflint to be installed.

I realise installing packages using a postCreateCommand script can work, but it places the onus on the developer to know what features to include to install the given package. For example, I'm building features which install Python packages (e.g. mkdocs) for C# .Net developers, who wouldn't know to include the python feature.

I imagine a devcontainer-feature.json file including a dependsOn property, in the same schema as the devcontainer.json features object.

I.e.

"dependsOn": {
  "ghcr.io/user/repo/go": {},
  "ghcr.io/user/repo1/go:1": {},
  "ghcr.io/user/repo2/go:latest": {},
  "https://github.com/user/repo/releases/devcontainer-feature-go.tgz": { 
        "optionA": "value" 
  },
  "./myGoFeature": { 
        "optionA": true,
        "optionB": "hello",
        "version" : "1.0.0"
  }
}
@joshspicer
Copy link
Member

Thank you for your insights. The extend in which we want to incorporate some notion of loose/strong dependencies (if any) has been a topic of discussion for a while now. This is a valuable perspective.

The reference implementation and proposed spec provides an installsAfter attribute that can be added to a feature's devcontainer-feature.json.

installsAfter is an array of ID’s of Features that should execute before this one. Allows control for feature authors on soft dependencies between different Features

@virtualstaticvoid
Copy link
Author

Ah, I wasn't aware of the installsAfter feature. Not sure how I missed that.

Thanks for the reply!

@joshspicer
Copy link
Member

Please feel free to update us on whether installsAfter works for your use case. We would love the feedback (good and bad!)

@virtualstaticvoid
Copy link
Author

Will do. I do have further feedback, related to installations into the user's home directory and also secrets, but I'll raise those with separate issues.

@virtualstaticvoid
Copy link
Author

Has installsAfter been implemented in cli?.

I updated my feature JSON and it doesn't seem to work as expected.

Surprisingly, a search of Github for installsAfter contains very few results; and without any code which suggests it is implemented.

@joshspicer joshspicer reopened this Sep 6, 2022
@joshspicer
Copy link
Member

Yep, looks like a small typo after some refactoring. We'll be pushing changes shortly, and then afterwards please let us know your feedback :)

@edgonmsft edgonmsft self-assigned this Sep 6, 2022
@Chuxel Chuxel transferred this issue from devcontainers/spec Sep 6, 2022
@Chuxel
Copy link
Member

Chuxel commented Sep 6, 2022

Transferred issue to CLI repo since this is a bug verses a spec change at the moment.

@Chuxel Chuxel added the bug Something isn't working label Sep 6, 2022
@Chuxel Chuxel changed the title Feature Dependencies Feature Dependencies - installsAfter not respected Sep 6, 2022
virtualstaticvoid added a commit to virtualstaticvoid/devcontainers-cli-issue-163 that referenced this issue Sep 12, 2022
@virtualstaticvoid
Copy link
Author

virtualstaticvoid commented Sep 12, 2022

Hello,

I tried running using the latest (HEAD) version and found that installsAfter still doesn't work.

I've created a test feature project, virtualstaticvoid/devcontainers-cli-issue-163 to replicate the issue.

It has a feature which installs a Python package using pip and specifies python in the feature's installsAfter.

The test feature's install.sh script fails, since pip is not installed, and the output shows that the python feature doesn't get installed.

Please take a look to see whether I'm doing something wrong, or whether my understanding of installsAfter is incorrect.

Thanks.

@Chuxel
Copy link
Member

Chuxel commented Sep 12, 2022

@edgonmsft ?

@edgonmsft
Copy link
Contributor

Hi @virtualstaticvoid

I just recently fixed an issue with the implementation of installsAfter but I think you might be running into a different situation. The current definition for installsAfter is that it just reorders the installation of the features defined on devcontainer.json but doesn't add them to the list.

Was that your understanding?

@virtualstaticvoid
Copy link
Author

Hi @edgonmsft

Ah ha! Yes, I assumed they would be added to the list automatically.

I suppose having the feature dependencies automagically added presents many more challenges, so I understand now why it's implemented as a soft dependency instead.

I've updated my example feature's test to use a scenarios.json to work around the "default test" scenario which of course won't work.

I have one scenario to include the python feature, but devcontainer features test ... fails as can be seen in the test output as it isn't installing the python feature as part of the test.

@virtualstaticvoid
Copy link
Author

^^ For the test changes see PR devcontainers/features#1

@edgonmsft
Copy link
Contributor

I think for this one the name in installsAfter has to be fully qualified. But let me make a test.

@Chuxel
Copy link
Member

Chuxel commented Sep 29, 2022

@edgonmsft Did we come to a conclusion on this one?

@edgonmsft
Copy link
Contributor

Hi! Sorry for the delay responding here.
I tested the repository and found a couple of things:

  • We only take into account the scenarios.json for global tests. those have to be under a _global folder inside test. The default.sh file also goes there.
  • For installsAfter to be respected it has to be a fully qualified reference, for your case an example could be: ghcr.io/devcontainers/features/python
  • Even after this two changes I still find a bug that needs fixing which is that the current scenarios only test features inside the repo, they don't take into account bringing features from OCR.

cc @joshspicer, @Chuxel, @samruddhikhandale, @chrmarti, @alexdima on your opinions on the best way forward for this scenario.

@Chuxel
Copy link
Member

Chuxel commented Oct 5, 2022

Ok - so:

  1. There's clearly a bug with the test command here we should fix since I can see people wanting to be able to do this. There are some other gaps like being able to use remoteUser.
  2. ...and some missing docs on what is / is not expected to work.
  3. We should probably raise a spec issue on the idea of a "composite" feature that can automatically pull in the dependency rather than affecting order if it is referenced.

@johnnyreilly
Copy link

Just linking devcontainers/spec#109 which looks like it covers similar ground to this

@Chuxel
Copy link
Member

Chuxel commented Oct 6, 2022

Just linking devcontainers/spec#109 which looks like it covers similar ground to this

Thanks for pointing that out - I moved that issue to the spec repo for discussions on the composite topic.

@edgonmsft
Copy link
Contributor

Hey @virtualstaticvoid just to let you know that @joshspicer just merged a change that should support your scenario.

@joshspicer
Copy link
Member

To @Chuxel 's (2) above, working on documentation for this command :) Feel free to provide input if you'd like:

#219

@virtualstaticvoid
Copy link
Author

Hi @edgonmsft

Okay, I've updated my test repo with the changes to installsAfter and to use a global scenario test as mentioned above.

🎉 The global scenario test is passing now; installing the python feature dependency prior to running the "pip" test.

Thanks!

@edgonmsft
Copy link
Contributor

Awesome! Keep the feedback coming!

@rzhao271
Copy link
Contributor

rzhao271 commented Oct 27, 2022

As part of endgame, I tried running through the readme in https://github.com/virtualstaticvoid/devcontainers-cli-issue-163, but I encountered the following issue when trying to run the tests inside of the devcontainer:

devcontainer features test . \
  --base-image mcr.microsoft.com/devcontainers/base:ubuntu-22.04 \
  --global-scenarios-only

┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┐
|    dev container 'features' |   
│           v0.22.0           │
└ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┘


📊 Scenarios:         /workspaces/devcontainers-cli-issue-163/test/_global

>  Running scenario:  pip-test
>  workspaceFolder:   /tmp/devcontainercli/container-features-test/1666898460355

⏳ Building test container...

[0 ms] @devcontainers/cli 0.22.0. Node.js v19.0.0. linux 5.10.102.1-microsoft-standard-WSL2 x64.
[-] Failed to launch container:

Command failed: docker ps -q -a --filter label=devcontainer.local_folder=/tmp/devcontainercli/container-features-test/1666898460355

Are there alternative verification steps for this issue?
Or, @virtualstaticvoid, is the issue resolved for you on v0.22.0?

@rzhao271 rzhao271 removed the verified label Oct 27, 2022
@virtualstaticvoid
Copy link
Author

Hi @rzhao271

Only running global scenarios worked, as can be seen in the automated tests in the repository.

I had to explicitly remove running individual feature test, which explains the error you are getting.

@rzhao271
Copy link
Contributor

It could just be my setup.
I'll mark the issue as verified based on #163 (comment).
If the fix regresses, don't hesitate to comment on this issue or file a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working verified
Projects
None yet
Development

No branches or pull requests

7 participants