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 option to provide Play configuration from build.sbt #5

Merged
merged 8 commits into from
Apr 12, 2021

Conversation

bursauxa
Copy link
Contributor

@bursauxa bursauxa commented Apr 6, 2021

Aims to solve #3 and #4 🎉

At this point the PR is pretty much a fingers-crossed venture, because I was unable to run the tests locally. I had to change the signature of SwaggerRunner.run by adding one argument, I performed the change in both places (definition and call site) but the scripted tests do not seem to like that:

[info] [error] java.security.PrivilegedActionException: java.lang.NoSuchMethodException: com.github.dwickern.swagger.SwaggerRunner$.run(java.io.File, java.lang.String, boolean, scala.Option)
...
[info] [error] Caused by: java.lang.NoSuchMethodException: com.github.dwickern.swagger.SwaggerRunner$.run(java.io.File, java.lang.String, boolean, scala.Option)

Do you have any insight to help me run the tests @dwickern ?

@dwickern
Copy link
Owner

dwickern commented Apr 6, 2021

Having a quick look at your changes, I suspect you have that error because you're using Option[Map[String, Any]].

Sbt's build-time classloader is different from the classloader used to run the application (or in this case, generate the swagger spec). On top of that, they're typically running different binary-incompatible scala and scala-library versions. I believe the latest sbt is running scala 2.12 and most of us are using scala 2.13 for our applications.

I think the only solution is to avoid any scala types in that signature and stick to plain old Java classes. I think a nullable java.util.Map would work.

@bursauxa
Copy link
Contributor Author

bursauxa commented Apr 6, 2021

Hello, thanks for your answer!

I came to the exact same conclusion, and now have all tests passing. Thinking the generics could be the problem, I also tried passing a Configuration parameter directly, but could not make it work either. As you said, plain old Java classes are probably the safe choice here.

The test I added replicates the configuration from the existing one, but in build.sbt instead of application.conf - output is unchanged, so that's looking great.

@dwickern dwickern force-pushed the feature/configuration-from-code branch from 6bcafe1 to f2b95b7 Compare April 9, 2021 22:03
@dwickern
Copy link
Owner

dwickern commented Apr 9, 2021

I added cache invalidation so that we will re-generate swagger.json when swaggerPlayConfiguration changes. I'll run some tests and I think this is ready to merge

@bursauxa
Copy link
Contributor Author

That makes sense!

Just one thing, the Play secret key was here because I copied it from application.conf in the configured test - even though I found it surprising 😅 I wanted to have the exact same content.
If it is not required, as it looks from your change, then it should be removed from that application.conf as well.

@dwickern
Copy link
Owner

Right, the secret key is only needed for runProd

@dwickern dwickern merged commit 2d849e4 into dwickern:master Apr 12, 2021
@dwickern
Copy link
Owner

Released version 0.4.0. Thanks! 🎉

@bursauxa
Copy link
Contributor Author

My pleasure! Just updated the dependency in one of our projects and it solves our issues 😃

At the same time I noticed I messed up the changelog 😅 I opened a new PR to fix that.

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