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

Make recommended changes to our module manifests (breaking change for v0.23) #11482

Closed
StephenWeatherford opened this issue Aug 11, 2023 · 5 comments · Fixed by #12057
Closed
Assignees
Labels
breaking change P1 This is planned to be completed before the end of a release
Milestone

Comments

@StephenWeatherford
Copy link
Contributor

Definitely do:

  1. allow a non-empty config and an unrecognized config media type when we download a module (currently we throw)
  2. allow multiple module layers when we download (currently we throw)

Consider (would break previous Bicep versions from being able to download newly-published modules, including those in the public registry):

  1. Write "{}" as the config contents
  2. Change config media type to application/vnd.oci.empty.v1+json?
  3. We could consider a phase out with warning approach...

Context:

I discovered that ACR requires the sources manifest config to be valid JSON or it won't recognize it as an attachment. Upon asking the ACR guys if this was expected, I got this answer:

Hey Stephen, empty manifest support is somewhat inconsistent industry wide. Configs have only recently become that free as of the most recent RC3 image spec and we have not yet implemented support for that at the config level. I'll make sure to add it as a workitem nonetheless, thanks for bringing it to our attention. Regardless we would not suggest for the use of actually empty configs or layers for that matter though since it would not be portable as different registries have different levels of support for empty layers/config. The general guidance is to use the empty descriptor https://github.com/opencontainers/image-spec/blob/main/manifest.md#guidance-for-an-empty-descriptor as you have done in the example above if no config is wanted. RC3 Also allows for the complete removal of the config field but we haven't completed support for that (Its an ongoing change one of my colleagues is working on though so that is also an option in the future). P.S did you observe this behaviour only with the empty config or also with a binary one?

@shenglol
Copy link
Contributor

We will need to emit a warning about the breaking change as the first step.

@shenglol shenglol added discussion This is a discussion issue and not a change proposal. and removed Needs: Triage 🔍 labels Aug 16, 2023
@StephenWeatherford
Copy link
Contributor Author

related branch: sw/allow-multiple-layers

@StephenWeatherford
Copy link
Contributor Author

The ACR guys suggested the following changes:

{
"schemaVersion": 2,
"mediaType": "application/vnd.oci.image.manifest.v1+json", <<<<<<<<<<< ADDED
"artifactType": "application/vnd.ms.bicep.module.artifact",
"config": {
"mediaType": "application/vnd.ms.bicep.module.config.v1+json",
"digest": <digest of {}>, <<<<<<<<<<<< CHANGED
"size": 0, <<<<<<<<<<<<<<< CHANGED FROM 0 TO 2 (pointing to "{}" blob)
"annotations": {}
},
"layers": [
{
"mediaType": "application/vnd.ms.bicep.module.layer.v1+json",
"digest": "sha256:fa703356d827467cede2e38c00501fc9fc8f66eba3e6e13a0a3f37adae50528a",
"size": 117570,
"annotations": {}
}

@StephenWeatherford StephenWeatherford changed the title we should considering always pushing an OCI manifest config blob with "{}" rather than empty Make recommended changes to our module manifests (will eventually be breaking change) Aug 29, 2023
@StephenWeatherford StephenWeatherford added breaking change and removed discussion This is a discussion issue and not a change proposal. labels Aug 29, 2023
@StephenWeatherford
Copy link
Contributor Author

StephenWeatherford commented Aug 29, 2023

SUMMARY OF CHANGES

Now

Breaking change in v0.23 - see PR for for breaking change details)

@puicchan puicchan modified the milestones: v0.21, v0.22 Aug 29, 2023
StephenWeatherford added a commit that referenced this issue Sep 5, 2023
Part of the work for #11482. These
changes do not cause any breaking changes, but they set us up for doing
breaking changes after a release or two, to make it easier for people to
switch to newer versions.

All changes are non-breaking:
- allow a non-empty config and an unrecognized config media type when we
download a module (currently we throw)
- allow multiple module layers when we download (currently we throw)
- change manifest media type to
application/vnd.oci.image.manifest.v1+json instead of null

With these changes, the upcoming breaking changes will only break
v-minus-2 (v0.20) instead of breaking v-minus-1. Assuming the breaking
changes occur in v0.22:
- Current version (v0.20) or earlier: Will get an error trying to use a
module published with v0.22
- v0.21 (Early September): Can use modules published with v0.22 (but no
new functionality)
- v0.22 (Early October): Breaking changes made affecting v0.20 but not
v0.21


###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/11680)

---------

Co-authored-by: Stephen Weatherford <Stephen.Weatherford.com>
@StephenWeatherford StephenWeatherford modified the milestones: v0.21, v0.22 Sep 6, 2023
@StephenWeatherford StephenWeatherford added the P1 This is planned to be completed before the end of a release label Sep 7, 2023
@StephenWeatherford StephenWeatherford modified the milestones: v0.22, v0.23 Sep 27, 2023
@StephenWeatherford StephenWeatherford changed the title Make recommended changes to our module manifests (will eventually be breaking change) Make recommended changes to our module manifests (breaking change for v0.23) Oct 5, 2023
@StephenWeatherford
Copy link
Contributor Author

See #12057 for breaking change details

StephenWeatherford added a commit that referenced this issue Oct 5, 2023
Fixes #11482

This is a breaking change that keeps v0.20 or earlier from restoring
modules published with versions with this change.

The original breaking change notification doesn't seem to have made it
into the release notes, but we can put this into the new version:

> This is a breaking change in how modules are published. This change
will affect v0.20 and earlier, keeping them from being able to restore
modules created in v0.23 or later.
> v0.20 or earlier: Can read modules published with v0.22 or earlier.
Will get an error trying to restore a module published with v0.23 or
later.
> v0.21, v0.22: Can read modules published earlier or later. Modules
published with v0.21 can be read by earlier and later versions.
> v0.23: Can read modules published earlier or later. Modules published
with v0.23 can be read by v0.21 and later.

###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/12057)

---------

Co-authored-by: Stephen Weatherford <Stephen.Weatherford.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change P1 This is planned to be completed before the end of a release
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants