-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add devcontainers
ecosystem
#8445
Add devcontainers
ecosystem
#8445
Conversation
730d14f
to
e64e20f
Compare
f010370
to
2bcda52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a first review pass, seems pretty neat!
devcontainers/spec/devcontainers/devcontainers/file_parser_spec.rb
Outdated
Show resolved
Hide resolved
devcontainers/lib/dependabot/devcontainers/file_updater/config_updater.rb
Show resolved
Hide resolved
devcontainers/lib/dependabot/devcontainers/file_parser/feature_dependency_parser.rb
Outdated
Show resolved
Hide resolved
devcontainers/spec/devcontainers/devcontainers/file_parser_spec.rb
Outdated
Show resolved
Hide resolved
37a83de
to
b1d6383
Compare
3f77e3e
to
35f9c51
Compare
35f9c51
to
fb46440
Compare
I rebased the PR, addressed several issues, and implemented the new way of handling updates. Now we provide updates to all fetched files, relative to the configured directory (by default "/"). For example:
I saw a few more issues that I'll be addressing next week:
Also found an issue in the CLI that I reported at devcontainers/cli#712. |
ed2a6ed
to
c422b65
Compare
I fixed all the issues mentioned previously and got CI to pass. However, I was thinking about the smart approach taken here of asking
So I think we still need to implement a basic We'll need to work on this after holidays! |
Alternatively, and probably better since it would only require changes to the devcontainers CLI, we could add an |
ba1c00d
to
9550e68
Compare
Found and fixed a few more issues, including one that I reported upstream (and added workaround for it here). Also cleaned up git history to keep things a bit more tidy. This is ready for a review! |
9698a46
to
c3a6297
Compare
c3a6297
to
2e37aa0
Compare
5c4d352
to
479b521
Compare
✅ Did some bug bashing on the changes and I see no release blockers! Some follow up fixes that I can explore moving forward, but I think given the new timeline this is good to go!
|
fd70cb9
to
fe6c46a
Compare
@joshspicer I looked at the issues you found:
I also fixed some other issues I found and added support for ignores! |
10a97b2
to
5f04f7b
Compare
I merged the first commit in this PR as #8858, so that I can build updater images for this PR (since workflow files are picked up from the main branch). |
Co-authored-by: Josh Spicer <[email protected]>
Co-authored-by: Josh Spicer <[email protected]>
This class at the moment just delegates to the FileUpdater, because of the heavy lifting is done by the FileParser class, since the `devcontainers outdated` command provides both current dependency and available updates information. Co-authored-by: Josh Spicer <[email protected]>
Co-authored-by: Josh Spicer <[email protected]>
For example, ``` bin/dry-run.rb devcontainers devcontainers/images --dir /src/go ``` would update manifests with the wrong content because it was taking the contents of the manifests in root as the updated content.
5f04f7b
to
ae771dc
Compare
Let's do this! |
This change provides initial support for a devcontainers ecosystem, leaning heavily on the
devcontainers/cli
for ecosystem-specific implementation.This updater will look in the provided
--directory
for adevcontainer.json
(or.devcontainer.json
ifdirectory === /
) and update all dev container Features that are pinned to an out-of-date major version. The lockfile will also be updated if one is already present in the target directory.I aim to add future updater granularity/modes in the future.
Requires at least dev container CLI v0.54.0:
Demos (with the
dependabot/cli
)Closes #7000.