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

Changes the config layer of the feature manifest to a empty descriptor #815

Merged

Conversation

ych-tnk
Copy link
Contributor

@ych-tnk ych-tnk commented Apr 30, 2024

Description

This commit changes the config layer of the feature manifest to a empty descriptor according to the OCI image manifest specification to to increase the available feature registry.

Related Issue

Issue #814

Motivation and Context

Some registries that implement the OCI image manifest specification, such as AWS ECR, do not support pushing empty content (size 0). However, the manifest may contain layers with no content. Therefore, the OCI Image Manifest specification specifies an empty descriptor of non-zero size to represent a layer with no content.

Currently, the devcontainer feature manifest specifies the config layer as an empty content (size 0) layer. If the devcontainer feature manifest conforms to this specification, it can also available registries that cannot publish empty content (size 0).

Specification: https://github.com/opencontainers/image-spec/blob/8f3820ccf8f65db8744e626df17fe8a64462aece/manifest.md#guidelines-for-artifact-usage

Note: If the config is set to an empty descriptor, the manifest media type must be defined in the artifactType.

How Has This Been Tested?

  • Manually test that the following operations succeed with the modified code:
    • Publish the new feature to the AWS ECR
    • Publish the new feature to the GitHub Registry
    • Add the new feature published to the AWS ECR using the modified code to features and up the devcontainer
    • Add the features published to the GitHub Resigtry using the unmodified code to features and up the devcontainer

This commit changes the config layer of the feature manifest to a empty descriptor according to the OCI image manifest specification to to increase the available feature registry.

Some registries that implement the OCI image manifest specification, such as AWS ECR, do not support pushing empty content (size 0). However, the manifest may contain layers with no content. Therefore, the OCI Image Manifest specification specifies an empty descriptor of non-zero size to represent a layer with no content.

Currently, the devcontainer feature manifest specifies the config layer as an empty content (size 0) layer. If the devcontainer feature manifest conforms to this specification, it can also available registries that cannot publish empty content (size 0).

Specification: https://github.com/opencontainers/image-spec/blob/8f3820ccf8f65db8744e626df17fe8a64462aece/manifest.md#guidelines-for-artifact-usage

Note: If the config is set to an empty descriptor, the manifest media type must be defined in the artifactType.
@ych-tnk ych-tnk requested a review from a team as a code owner April 30, 2024 07:34
Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Looping in @joshspicer for his expertise with OCI spec changes.

src/spec-configuration/containerCollectionsOCIPush.ts Outdated Show resolved Hide resolved
@ych-tnk
Copy link
Contributor Author

ych-tnk commented May 10, 2024

@microsoft-github-policy-service agree company="freee"

The content of the configuration layer of the feature manifest is specified as empty content (size 0).
This is because there is no content to reference in the config layer content of the feature manifest.

However, some registries that implement the OCI image manifest specification, such as AWS ECR, do not
support pushing empty content (size 0).
Therefore, this commit changes the content of the config layer of the feature manifestto indicate that
empty objects have no content to reference.
@ych-tnk ych-tnk force-pushed the change-config-layer-to-empty-descriptor branch from 4b379ae to a83bb7e Compare May 10, 2024 16:56
Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

@ych-tnk Can you help fix the failing test? thanks!

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.

The changes here look good to me - thanks! You've tested against AWS's container registry with the very latest changes?

For the failing tests looks like you'll want to revert those hashes back (since blobs with those hashes aren't yet pushed to the registry)

If you want to further exercise via tests, you may want to consider expanding upon the tests here, which runs a locally-hosted container registry instead of depending on previously-published Features.

@ych-tnk ych-tnk force-pushed the change-config-layer-to-empty-descriptor branch from 0abc8b8 to 192abc4 Compare June 1, 2024 02:11
@ych-tnk
Copy link
Contributor Author

ych-tnk commented Jun 1, 2024

@joshspicer

You've tested against AWS's container registry with the very latest changes?

I updated the branch and tested pushing and installing the feature again using the AWS Container Registry without any issues.

If you want to further exercise via tests, you may want to consider expanding upon the tests here, which runs a locally-hosted container registry instead of depending on previously-published Features.

I don't know how to run the GitHub registry locally, so I decided to revert the test changes. After this PR is merged and the feature that the test depends on is pushed, I will create a PR that will change to depend on the new version of the feature. 192abc4

@joshspicer
Copy link
Member

I don't know how to run the GitHub registry locally,

Just for clarity, the tests I linked to interact directly with a "local" registry in a docker container (I bit meta, I know). Nothing ever leaves the test infrastructure, so it's a nice place to write tests since it's all "emulated" in containers instead of hitting a real registry

@joshspicer
Copy link
Member

I used your changes here to publish to ghcr.io as a final smoke test 🚀

I pushed these Features:
https://github.com/orgs/codspace/packages?tab=packages&q=non-empty

And consumed the Feature in this test repo:
https://github.com/codspace/non-empty-config-layer-project/blob/main/.devcontainer/devcontainer.json

I don't see any concerns with older CLI backwards compatibility 👍

@ych-tnk
Copy link
Contributor Author

ych-tnk commented Jun 4, 2024

@joshspicer

Since the test was failing, I changed the tests to depend on the package you created for the smoke test. If there seems to be a problem, I will replace the test with a local registry on the docker container. d134da7

@joshspicer
Copy link
Member

Given that you're now just wrestling with tests that i've set up, i'm happy to take over the PR to get it over the line :) Appreciate all the work and communication!

@joshspicer joshspicer merged commit 7c2d300 into devcontainers:main Jun 5, 2024
19 checks passed
@ych-tnk ych-tnk deleted the change-config-layer-to-empty-descriptor branch June 5, 2024 18:33
joshspicer added a commit that referenced this pull request Jun 6, 2024
I forgot to include #815 in the changelog
joshspicer added a commit that referenced this pull request Jun 6, 2024
I forgot to include #815 in the changelog
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