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

sbt plugin for OpenAPI Generator #5406

Conversation

chameleon82
Copy link
Contributor

@chameleon82 chameleon82 commented Feb 23, 2020

Regarding to #3650 sbt plugin for OpenAPI Generator added

Few concerns about this PR:

  • need to define which repositories this artefact will be published. At least, for sbt community it should be published to https://bintray.com/sbt/sbt-plugin-releases, I believe @eed3si9n can help us on it. (continued here Set up repositories for publishing releases and snapshots sbt-openapi-generator#5)
  • plugin name convention. Following OpenApi artefact should be named as openapi-generator-sbt-plugin, but sbt plugin best practices recommends to name it as sbt-openapi-generator.
    Resolution: sbt-openapi-generator as separate project
  • settings name convention. Right now all settings haven't prefix, but sbt has only global domain for all settings which can be conflicted with other sbt plugins. Should we change it following sbt best practices? Sbt recommends to reuse default settings if possible, but not much settings we can. Alternate way to keep original names for settings is to put all openapi settings in separate openapiSettings case class, but in this case sbt will loose description of each setting (which probably ok)
    Resolution: openApi prefix
  • maven integration. As i understand it is possible to build plugin with maven (sbt integration tests still require sbt anyway) and integrate this module as part of project, but have no experience on it.
    Resolution: don't needed as moved to separate project
  • should plugin support src_managed directory for generated files?
    Resolution: up to user. will be better described in README

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the (technical committee)[https://github.com/openapitools/openapi-generator/Better badge description #62---openapi-generator-technical-committee] to review the pull request if your PR is targeting a particular programming language.

@jimschubert @wing328 @YanDoroshenko

@jimschubert
Copy link
Member

@chameleon82 I think this looks great. I wonder if it would be better to include this as a separate repository under OpenAPI Tools? I think that would address some concerns that might arise from adding another plugin here:

  • We can follow SBT Plugin best practices, such as not conflicting with the Maven/Gradle naming conventions
  • We can configure deployments in a way that don't interfere with our Maven/Sonatype deploys (to avoid confusion that we've had with like the Gradle POM wrapper)
  • We don't have to integrate with Maven. I'd much rather stick with SBT-only in this case.
  • We won't introduce additional build time to the core project

I think the separate repo would be good as well because it would allow us to setup GitHub Workflows for build/release. We could also create a cron workflow which checks OpenAPITools/openapi-generator daily for new releases and automatically prepare a new release Pull Request of the SBT Plugin. I'm planning to do something similar for my IntelliJ Plugin as well.

As far as your other questions, I think an openApi prefix on settings makes sense. Also, I'm not sure about the src_managed question… are you asking if we should default the output directory to (sourceManaged in Compile).value / "out"? I think it would be a good default but best to leave this up to the user, maybe just documenting it in the README.

I have a few questions/comments about the implementation. Please forgive me, because it's been about 2.5 years since I last write an SBT plugin.

  • In trait OpenApiCodegen, you create a new configuration called OpenApiCodegen. I thought it was considered bad practice to create a new config in most cases, is there a specific reason for this or could we extend another config or even simplify as val OpenApiCodegenConfig = config("openapi")?
  • For defaults, I think we'll also need to separate some like Compile and Test, for example:
      keyForOutputDir in Compile := (sourceManaged in Compile).value / "out",
      keyForOutputDir in Test := (sourceManaged in Test).value / "out",
    
  • Usually we'd see an exported sequence of settings which define the keys and their defaults for the plugin, and these are applied to the configuration specifically (see scrooge for example). Is that done differently here because of the Config file support?

@chameleon82
Copy link
Contributor Author

@jimschubert i've implemented new config as sbt doesn't have default Generate or something like this.
I considered next scenario:

  • able to build sources as separate project(module) with Codegen config
  • compile project/module with generated files
  • keep generated files in git to able track changes
    Make generation under Compile config also make sense in most cases and i think it possible to achieve above goals with Compile config. Will try to rethink about usage scenarios.
  • yes, settings applied differently because of config file support. It make difficult to override value when default value presents in config as need to apply config file settings first to configurator and then check if setting value not equal to default and not equal to configurator value and have to repeat it for every supported configuration. It will be easier to manage if codegen default settings will be public available. For now most values are private.

@chameleon82
Copy link
Contributor Author

@jimschubert revised the code, seems OpenApiCodegen config is redundant. I deleted it and renamed settings with prefix.
sourceManaged seems a bit complicated, i tried to use options

Compile / sourceGenerators += openApiGenerateTask.taskValue.map{ 
  _.map{ file => file.filter(...).getAbsoluteFile }
}

and

val generatedSources = mappings in (Compile, packageSrc) ++= { ... }

But as main purpose of plugin is to generate whole project it unclear for me how to reload dependencies from generated build.sbt file at compilation time.
It could be simple as sbt "openApiGenerate; reload; compile" in build flow.

sourceManaged makes sense when user don't want to generate build.sbt and use predefined instead. In that case configFile.outputDir setting must be ignored or sbt openApiOutputDir setting changed to a task. But i got confused to separate it from normal project compilation and need help to implement this scenario.

@jimschubert
Copy link
Member

@chameleon82 I've setup https://github.com/OpenAPITools/sbt-openapi-generator

Should we move your work there?

@jimschubert
Copy link
Member

Closing this as we created https://github.com/openapitools/sbt-openapi-generator

I'll leave #3650 to address documenting the new plugin

@chameleon82 chameleon82 deleted the openapi-generator-sbt-plugin branch May 25, 2020 07:20
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