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

Initial version of OCI packages for features. #94

Merged
merged 24 commits into from
Aug 3, 2022
Merged

Initial version of OCI packages for features. #94

merged 24 commits into from
Aug 3, 2022

Conversation

edgonmsft
Copy link
Contributor

The code here implements the latest changes to the features proposal as written here:

devcontainers/spec#66

The code parses the feature id and decides its origin between the new OCI features and old github release based features by verifiying a manifest is available and reachable for said feature.

The main TODO is implementing authentication support for the various operating system keychains and obtain the docker credentials.

@edgonmsft edgonmsft marked this pull request as ready for review August 1, 2022 15:04
@edgonmsft edgonmsft requested a review from a team as a code owner August 1, 2022 15:04
joshspicer
joshspicer previously approved these changes Aug 1, 2022
Copy link
Member

@joshspicer joshspicer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, super excited for this! We just gotta make sure the test is properly fetching (and once we open source devcontainers/features, can point the tests to those features.)

src/spec-configuration/containerFeaturesConfiguration.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesConfiguration.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesConfiguration.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesOCI.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesOCI.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesOCI.ts Outdated Show resolved Hide resolved
joshspicer
joshspicer previously approved these changes Aug 1, 2022
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! I left a few comments, and one more general one would be that we might consider adding some simple retry logic to these https requests.

src/spec-configuration/containerFeaturesOCI.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesConfiguration.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesOCI.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesOCI.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesConfiguration.ts Outdated Show resolved Hide resolved
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left more comments

src/spec-configuration/containerFeaturesOCI.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesOCI.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesOCI.ts Outdated Show resolved Hide resolved
src/spec-configuration/containerFeaturesOCI.ts Outdated Show resolved Hide resolved
@joshspicer joshspicer mentioned this pull request Aug 2, 2022
@edgonmsft
Copy link
Contributor Author

LGTM

Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

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.

3 participants