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

Add support for JSON serialization #507

Merged
merged 5 commits into from
Feb 16, 2024
Merged

Conversation

mdanish-kh
Copy link
Contributor

@mdanish-kh mdanish-kh commented Feb 5, 2024

Changes

  • Add a new CLI arg --format to specify output manifest format (json, yaml etc).
  • Added a new user setting to specify the manifest format (inline --format arg takes precedence over this value)
  • Refactored current Serialization.cs for future extension of other serializations
  • Added check to only allow YAML submissions to community repository
  • Implemented support for deserialization from local files / GitHub repo contents of JSON format. Refactored GitHub flow to allow submission of json manifests.

Validations

Added E2E tests for JSON, and added a bunch of unit tests.

Important

The tests will only pass once changes in microsoft/winget-pkgs-submission-test#4784 are merged since I had to refactor the directory structures as well. For local testing, you can pull the changes in that PR to the master branch of your fork and run the tests locally;

cc @denelon @Trenly from the original issue for any naming/design considerations


Microsoft Reviewers: Open in CodeFlow

@mdanish-kh mdanish-kh requested a review from a team as a code owner February 5, 2024 20:32
@mdanish-kh mdanish-kh requested review from yao-msft and ryfu-msft and removed request for a team February 5, 2024 20:32
@microsoft-github-policy-service microsoft-github-policy-service bot added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Feb 5, 2024
@denelon
Copy link
Contributor

denelon commented Feb 12, 2024

wingetcreate doesnt' necessarily need to serialize as JSON. YAML serialization is fine as long as wingetcreate knows how to transform the YAML file-based manifests to the JSON expected by a WinGet source REST endpoint.

The same would be true in terms of the upgrade scenario where the package ID and source are REST based. The transformation from JSON to YAML could happen inside wingetcreate.

@denelon
Copy link
Contributor

denelon commented Feb 12, 2024

I'm not opposed to this, but we should be careful in terms of how we decide which format wingetcreate defaults to and how we do any of the transformations.

@mdanish-kh
Copy link
Contributor Author

mdanish-kh commented Feb 12, 2024

wingetcreate will always default to YAML serialization. That is the default --format value and also the default user setting value.

Moreover, submitting anything other than YAML to the main community repository has also been blocked as part of this PR.

I'm not opposed to closing this if we don't want this, but there could still be value in this PR. The refactorings made to the Serialization flow here may make it easier for a future REST-source implementation. The target of this PR was not really REST implementations but more of private GitHub repositories that want JSON as their output manifests (as was mentioned in the original feature request)

@denelon
Copy link
Contributor

denelon commented Feb 12, 2024

No worries :)

If @ryfu-msft is good with it, then I am too.

@ryfu-msft
Copy link
Contributor

@mdanish-kh, will take a look at this soon.

I want to create a new release for winget-create to support 1.6 manifests first since there are a lot of your new features in there. This will be included in our next 1.7 release.

@ryfu-msft ryfu-msft self-assigned this Feb 14, 2024
@ryfu-msft ryfu-msft added this to the v1.7 milestone Feb 14, 2024
@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft
Copy link
Contributor

I have merged microsoft/winget-pkgs-submission-test#4784 so that these tests will pass. We will need to get this in first so that #512 can pull in latest changes.

Copy link
Contributor

@ryfu-msft ryfu-msft left a comment

Choose a reason for hiding this comment

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

Really awesome feature :)

@ryfu-msft ryfu-msft merged commit e741605 into microsoft:main Feb 16, 2024
5 checks passed
@mdanish-kh mdanish-kh deleted the json branch February 16, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support creating manifests as JSON
3 participants