Skip to content

Conversation

@wing328
Copy link
Contributor

@wing328 wing328 commented Jun 12, 2018

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@ilovezfs
Copy link
Contributor

Can we not use http://central.maven.org/maven2/org/openapitools/openapi-generator/3.0.1/openapi-generator-3.0.1.jar ?

@wing328
Copy link
Contributor Author

wing328 commented Jun 12, 2018

@ilovezfs technically yes. I'm not sure why the formula (swagger codegen) was created with compiling the project (tar.gz) in the first place.

Would it be ok to merge it first so that Brew users can start using it and we'll try a separate PR to enhance the formula by using the JAR directly?

@jimschubert
Copy link

@wing328 I think swagger-codegen supported installing "head" and would therefore need to recompile code directly. I think it's fine to lock openapi-generator down to only jars officially published to Maven.

@wing328
Copy link
Contributor Author

wing328 commented Jun 12, 2018

@jimschubert I prefer we provide the exact same formula for openapi-generator so that users can do the same (e.g. installing head) with openapi-generator. (from time to time we ask users to try the latest master to see if the fix works for them)

@SMillerDev
Copy link
Member

new formula aren't supposed to have HEAD options though. So that won't work with a new formula

@wing328
Copy link
Contributor Author

wing328 commented Jun 12, 2018

Btw, this is the formula added a few years ago: https://github.com/arnested/homebrew-core/commit/69aa71155e4e317076f8c45e17197db63b305128 (which has HEAD defined)

If new formula is not supposed to have HEAD options, how can we add it (later?) ?

We want to make both swagger-codegen and openapi-generator almost identical in terms of usage.

@jimschubert
Copy link

@wing328 I wasn't suggesting we never have head builds. Just explaining why the recompile existed in swagger-codegen, and that pulling the pre built jar is both faster and more secure (jars are signed, head branches aren't). So my preference on default install is to use the jar. When we re-introduce head, I think we should recompile head only conditionally. But I agree with you that the simple approach of compiling directly should be acceptable, considering we'll want to follow up later with head support.

Sorry if my previous comment was confusing.

Question: what are the limits before we support the head instruction? Like is there an age of 30 days as there is with repos for new formulas?

@wing328
Copy link
Contributor Author

wing328 commented Jun 14, 2018

@ilovezfs may I know how we can move this forward?

Must this formula download the JAR since it's a new formula? And then later we can switch to "swagger-codegen" (--HEAD) approach to allow compiling from the latest master?

@ilovezfs
Copy link
Contributor

I don't mind waiving the no-HEAD audit here. See the spotbugs formula for how to do that.

@wing328
Copy link
Contributor Author

wing328 commented Jun 16, 2018

@ilovezfs thanks for the fix. I was about to push a fix based on the audit result.

@wing328
Copy link
Contributor Author

wing328 commented Jun 16, 2018

@ilovezfs I've updated the url based on the audit result. Please have another look when you've time. Thanks.

@commitay commitay added the new formula PR adds a new formula to Homebrew/homebrew-core label Jun 16, 2018
@wing328
Copy link
Contributor Author

wing328 commented Jun 16, 2018

CI reports the following errors (merge conflicts):

Error Message
failed: brew pull --clean https://github.com/Homebrew/homebrew-core/pull/28921
Stacktrace
        fatal: ref HEAD is not a symbolic ref
==> Fetching patch 
Patch: https://github.com/Homebrew/homebrew-core/pull/28921.patch
==> Applying patch
Applying: openapi-generator-3.0.0
Applying: fix problem reported by jenkins
Applying: remove bottle as that should be added after the 1st release
Applying: remove head
Applying: openapi-generator-3.0.1
Applying: update formula based on feedback
Applying: Update openapi-generator.rb
Applying: update based on audit result
Using index info to reconstruct a base tree...
M	Formula/openapi-generator.rb
error: Failed to merge in the changes.
Falling back to patching base and 3-way merge...
Auto-merging Formula/openapi-generator.rb
CONFLICT (content): Merge conflict in Formula/openapi-generator.rb
Patch failed at 0008 update based on audit result
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: Patch failed to apply: aborted.

I'll submit a new PR instead with a new commit/branch instead to see if the issue goes away

@ilovezfs
Copy link
Contributor

please don't. we can fix it.

@wing328
Copy link
Contributor Author

wing328 commented Jun 16, 2018

@ilovezfs 👌 (I've just closed #29104)

@ilovezfs ilovezfs force-pushed the openapi-generator-3.0.1 branch from 057d749 to d7d6cc9 Compare June 16, 2018 08:10
@ilovezfs
Copy link
Contributor

iMac-TMP:homebrew-core joe$ openapi-generator generate -i spec -l openapi 
[main] WARN  o.openapitools.codegen.cmd.Generate - The '--lang' and '-l' are deprecated and may reference language names only in the next major release (4.0). Please use --generator-name /-g instead.
[main] INFO  o.o.c.ignore.CodegenIgnoreProcessor - No .openapi-generator-ignore file found.
[main] WARN  o.o.codegen.utils.URLPathUtils - Server information seems not defined in the spec. Default to http://localhost.
[main] WARN  o.o.codegen.DefaultCodegen - Empty operationId found for path: get /. Renamed to auto-generated operationId: rootGet
[main] WARN  o.o.codegen.utils.URLPathUtils - Server information seems not defined in the spec. Default to http://localhost.
[main] INFO  o.o.codegen.DefaultGenerator - writing file /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/./README.md
[main] INFO  o.o.codegen.AbstractGenerator - writing file /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/./.openapi-generator-ignore
[main] INFO  o.o.codegen.AbstractGenerator - writing file /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/./.openapi-generator/VERSION
[main] INFO  o.o.c.languages.OpenAPIGenerator - wrote file to /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/./openapi.json

It would be good for it to run without warnings, and certainly without using deprecated options.

@wing328
Copy link
Contributor Author

wing328 commented Jun 16, 2018

It would be good for it to run without warnings, and certainly without using deprecated options.

I agree. Is it ok to get this PR merged into master first and we'll file another PR to address the warning found in the test?

@ilovezfs ilovezfs merged commit 7bc3bd1 into Homebrew:master Jun 16, 2018
@ilovezfs
Copy link
Contributor

Yes, that is fine! Thanks for the 🆕 formula @wing328

@wing328
Copy link
Contributor Author

wing328 commented Jun 16, 2018

@ilovezfs thanks! Have a nice weekend.

@wing328 wing328 deleted the openapi-generator-3.0.1 branch June 16, 2018 10:19
@lock lock bot added the outdated PR was locked due to age label Jul 16, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants