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

[Kotlin] Add Option to Skip Merging Spec Files #19396

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

erikvanderwerf
Copy link
Contributor

@erikvanderwerf erikvanderwerf commented Aug 20, 2024

I have split my API spec YAML files across multiple files, and use the Gradle task to generate Kotlin server interfaces. The Gradle task incorrectly thinks it is UP-TO-DATE if only inputSpec is given because Gradle only checks that one file for changes, even though the generator is able to process the entire structure correctly. The inputSpecRootDirectory property is available which will tell Gradle to look at the entire directory, but for some reason (I don't know) the generator is unable to process the merged file. So I've just cut out the middle step, and allowed for the best of both worlds.

Introduced a new property inputSpecRootDirectorySkipMerge to conditionally skip the merging step of the specification files. I chose it to be similar to inputSpecRootDirectory so that it can be easily found with autocomplete. But I am certainly open to choosing a different property name :)

As said above, this has the very useful side-effect of allowing the user to specify inputSpecRootDirectory for its Gradle InputDirectory annotation, without running the merge step. This means the Gradle task will run if any files in the directory have changed, rather than just the inputSpec.

@wing328 @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m @stefankoppier

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 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (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.6.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 Aug 21, 2024

thanks for the PR

can you please add a test or 2 to cover the change?

@Nathanrown
Copy link

thanks for the PR.
I have the same request.

@wing328
Copy link
Member

wing328 commented Jan 8, 2025

@Nathanrown do you mind adding some tests by filing a new PR based on this one?

@erikvanderwerf
Copy link
Contributor Author

@wing328 the test setup would be fairly complex and I'm not familiar with the repository that well. Is there a test setup I can reference that merges multiple openapi files together?

@wing328
Copy link
Member

wing328 commented Jan 8, 2025

@erikvanderwerf is it correct to say that you've tested this change locally and confirmed it works for you?

run {
resolvedInputSpec = MergedSpecBuilder(inputSpecRootDirectoryValue, mergedFileName.getOrElse("merged")).buildMergedSpec()
logger.info("Merge input spec would be used - {}", resolvedInputSpec)
var runMergeSpec = true
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: can you please add comment explaining what runMergeSpec does?

you can copy from the description of this PR so that other contributors later reading code understand what problem/issue it's trying to solve.

@erikvanderwerf
Copy link
Contributor Author

erikvanderwerf commented Jan 13, 2025

I have been working on this to test some improvements, but I have been unable to replicate my original problem, despite being in the same repo on the same computer, just 4 months later. I suppose time really does heal all wounds...

My Gradle task to openApiGenerate is no longer being cached, on both 7.7.0 and 7.10.0, and the merged spec file is no longer being created. I am completely stumped as to why. @Nathanrown can you post any steps you were using to have Gradle cache the task?

Introduced a new property `inputSpecRootDirectorySkipMerge` to conditionally skip the merging step of the specification files. Updated the logic to honor this new property, ensuring merging only occurs if it is explicitly not skipped. Enabled configuration via Gradle build file.
@erikvanderwerf
Copy link
Contributor Author

My Gradle began caching the output again, so I was able to resume work here.

@wing328 this is a version I am happy with. I have been able to load the locally-built JAR for the Gradle plugin module and verify that the option is accepted as intended, and I am able to re-generate my controllers when any files in the spec directory are changed, while keeping up-to-date status when files are not changed, all without generating the intermediate merged spec file (which still does not work, for unknown reasons).

@wing328
Copy link
Member

wing328 commented Feb 19, 2025

good. let's give it a try

@wing328 wing328 merged commit 9374dbd into OpenAPITools:master Feb 19, 2025
16 checks passed
@wing328 wing328 added this to the 7.12.0 milestone Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants