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

Update swagger-parser to 2.0.1 #123

Merged
merged 1 commit into from
May 25, 2018
Merged

Update swagger-parser to 2.0.1 #123

merged 1 commit into from
May 25, 2018

Conversation

jmini
Copy link
Member

@jmini jmini commented May 22, 2018

Because 2.0.1 version of swagger-parser will be published soon (see swagger-api/swagger-parser#717) this PR give us a chance to test it in OpenAPI Generator before it is released.

@jmini
Copy link
Member Author

jmini commented May 22, 2018

@jimschubert:

I’d recommend against pushing a -SNAPSHOT dep to master. This will prevent us from being able to run git bisect to detemine when a regression was introduced.

This is a valid point. We can leave this PR open until the 2.0.1 release is published. This means that for testing we should do it on this branch.

@wing328
Copy link
Member

wing328 commented May 22, 2018

Definitely a valid point but I would suggest using the SNAPSHOT version of 2.0.1 so as to uncover potential issues before the official release. There were issues (regressions) upgrading to the latest swagger parser before and that's why we are using the latest SNAPSHOT version in the master to avoid running into similar issue.

@jmini
Copy link
Member Author

jmini commented May 22, 2018

We can not have both requirements:

  • Not having SNAPSHOT versions on master at all.
  • Being able to test now the next swagger-parser version before its release on master (this version is for the time being 2.0.1-SNAPSHOT).

I tend to agree with @jimschubert.


We will always have the case that we would like to test what will be impacted with the update of one of our dependencies. So we need to come up with a solution.

Maybe a solution would be to rename on this branch our version to 3.0.0.preview-SNAPSHOT or something like that, and to publish it on maven central snapshot.

From time to time (daily for example) we merge master branch into this branch.

We ask interested users to test the version 3.0.0.preview-SNAPSHOT of OpenAPI Generator.

This way we have a way to test 2.0.1-SNAPSHOT of swagger-parser but we keep very strong dependency policy on our master branch (that can help with regression investigation in the future).

@jimschubert
Copy link
Member

Would it be out of the question to commit a local lib of the SNAPSHOT every 1-2 days? This way we would have repeatable builds, and still be able to evaluate the changes across branches.

@jmini
Copy link
Member Author

jmini commented May 22, 2018

@jimschubert: I am sorry, but I do not understand what you mean...

@jimschubert
Copy link
Member

@jmini we can put the jar (and any transient SNAPSHOT jars) in a lib folder as unmanaged artifacts. They'd be in the class path and compiled into the shadow jar. I would need to see how to do this with maven and the plug-in.

@jmini
Copy link
Member Author

jmini commented May 23, 2018

@jimschubert: I do not think that this is the maven way of handling dependencies.

And yes, this will cause trouble with people testing the maven plugin (and overriding some dependencies in their pom).

And having jars (binaries) committed in a git repo does not seems right. It is ok for the gradle-wrapper (that is project independent and almost always the same) but not for dependencies.


  1. We allow SNAPSHOT on master
  2. We don’t test upstream dependencies
  3. We create this “preview” branch with published artifacts (p or preview suffix in the version name) that works parallel to master, where we can test some SNAPSHOT version of our dependencies.

Because not having "1." is important for later investigations, I propose to go with "3.".

@jmini jmini changed the title Update swagger-parser to 2.0.1-SNAPSHOT Update swagger-parser to 2.0.1 May 24, 2018
@wing328 wing328 added this to the 3.0.0 milestone May 25, 2018
@jmini jmini merged commit 16ff517 into master May 25, 2018
@jmini jmini deleted the update_parser branch May 25, 2018 04:19
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