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

[ASPNETCORE] Add support for central package version management #20192

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Urganot
Copy link

@Urganot Urganot commented Nov 26, 2024

This pr adds support for using centralized package version management.

For this two options were added:

  1. useCentralizedPackageVersionManagement Type: Boolean, Default: false
  2. centralizedPackageVersionManagementOptOut Type Boolean, Default: false

Setting useCentralizedPackageVersionManagement true will add the ManagePackageVersionsCentrally property to the csproj file, set the value to true and remove explicit versions from packages
Setting useCentralizedPackageVersionManagement true will add the ManagePackageVersionsCentrally property to the csproj file and set the value to false

Template files for both kinds of projects were updated accordingly.
A sample with useCentralizedPackageVersionManagement: true was added

Technical Committee mentions:
@mandrean @shibayan @Blackclaws @lucamazzanti @iBicha

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@wing328
Copy link
Member

wing328 commented Nov 28, 2024

https://github.com/OpenAPITools/openapi-generator/actions/runs/12040139303/job/33660370623?pr=20192

i don't think .net 9.0 is supported by the aspnetcore generator at the moment, and we welcome PRs with such enhancement.

@Urganot
Copy link
Author

Urganot commented Nov 28, 2024

https://github.com/OpenAPITools/openapi-generator/actions/runs/12040139303/job/33660370623?pr=20192

i don't think .net 9.0 is supported by the aspnetcore generator at the moment, and we welcome PRs with such enhancement.

That wasn't on purpose. It was left from my changes regarding .net9. I corrected it

Btw. can I set a PR to squash merge?

@Urganot
Copy link
Author

Urganot commented Nov 28, 2024

I noticed that the regenerated sample files are now using windows style line breaks and directory divider even in strings.
image
image

Do you know of a way to fix this?

@wing328
Copy link
Member

wing328 commented Nov 30, 2024

pushed a commit with updated samples and docs. let's see how that goes

@wing328
Copy link
Member

wing328 commented Nov 30, 2024

can you please review the build errors (e.g. https://github.com/OpenAPITools/openapi-generator/actions/runs/12092892132/job/33723209893?pr=20192) when you've time?

@Urganot
Copy link
Author

Urganot commented Nov 30, 2024

Done

@Urganot
Copy link
Author

Urganot commented Nov 30, 2024

The up to date check fails because of the window/linux thing
image

@wing328
Copy link
Member

wing328 commented Nov 30, 2024

do you have Windows Subsystem for Linux (WSL) installed (assuming you're using Windows)? you can try to run the script to update the samples under WSL.

@Urganot
Copy link
Author

Urganot commented Dec 1, 2024

Using WSL worked

{{#centralizedPackageVersionManagementOptOut}}
<ManagePackageVersionsCentrally>false</ManagePackageVersionsCentrally>
{{/centralizedPackageVersionManagementOptOut}}

Copy link
Member

Choose a reason for hiding this comment

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

can we simplify this to just one option (e.g. managePackageVersionsCentrally, which is a string, not boolean)?

{{#ManagePackageVersionsCentrally}}
    <ManagePackageVersionsCentrally>{{{.}}}</ManagePackageVersionsCentrally>
{{/ManagePackageVersionsCentrally}}

if it's set to false, add another additional properties skipVersion for below.

Copy link
Author

@Urganot Urganot Dec 2, 2024

Choose a reason for hiding this comment

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

If I understand you correctly that would mean when setting it to false it has a different behaviour than dont setting it at all?
Personally I dont like this. As a user I would expect that if I dont set it at all it would do the same as false. If you want to have only one option though, we could use a enum with "true", "opt-out", "default". Not setting it would use the default option

Copy link
Member

Choose a reason for hiding this comment

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

As a user I would expect that if I dont set it at all it would do the same as false

we can default the option to false

Copy link
Author

Choose a reason for hiding this comment

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

We could, but not having the property in the .csproj file and setting the property to false are very different things

Copy link
Member

Choose a reason for hiding this comment

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

OK

we could use a enum with "true", "opt-out", "default". Not setting it would use the default option

what about having 3 values in a single option instead as you suggested earlier?

Copy link
Member

Choose a reason for hiding this comment

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

Should be automatic

Copy link
Author

Choose a reason for hiding this comment

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

Could you take a look the the last commit? I cant figure out why it doesnt validate correctly

Copy link
Author

Choose a reason for hiding this comment

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

I tried everything I could think of but I still cant get it to validate that the value is one of the enums. How do we procede with this pr?

Copy link
Member

Choose a reason for hiding this comment

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

I'll try to find time this week to address the remaining piece and then get this PR merged to move it forward.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks :)

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.

2 participants