-
Notifications
You must be signed in to change notification settings - Fork 826
SCB-2017 Fixed the issue of inconsistent false positives after re-ser… #2014
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
Conversation
…ialization using swagger to parse different JDK versions.
public boolean isIgnoreSwaggerDifferent() { | ||
DynamicBooleanProperty property = | ||
DynamicPropertyFactory.getInstance() | ||
.getBooleanProperty("servicecomb.service.registry.instance.ignoreSwaggerDifferent", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestion.
service-registry/pom.xml
Outdated
<dependency> | ||
<groupId>io.swagger</groupId> | ||
<artifactId>swagger-models</artifactId> | ||
<version>1.5.22</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not add version, use dependency management if neccessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
|
||
private boolean isIllegalValue(String context) { | ||
if (context == null || context == "" || context.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use StringUtils.isEmpty from commons lang 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|| ServiceRegistryConfig.INSTANCE.isAlwaysOverrideSchema(); | ||
} | ||
|
||
private boolean isIllegalValue(String context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this method because should be is valid value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
scSchemaContent, | ||
localSchemaContent); | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
localSchemaEntry.getKey(), | ||
scSchemaContent, | ||
localSchemaContent); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block should executed when not equals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
</dependency> | ||
<dependency> | ||
<groupId>org.apache.servicecomb</groupId> | ||
<artifactId>swagger-generator-core</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestions
if (ServiceRegistryConfig.INSTANCE.isIgnoreSwaggerDifference()) { | ||
LOGGER.warn( | ||
"service center schema and local schema both are different:\n service center schema:\n[{}]\n local schema:\n[{}]" | ||
+ "\nYou need to increment microservice version before deploying. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
service center schema and local schema both are different:\n service center schema:\n[{}]\n local schema:\n[{}]"
+ "\nYou have configured to ignore difference check. It's recommented to increment microservice version before deploying when shcema change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
if (scSwagger.equals(localSwagger)) { | ||
return true; | ||
} | ||
if (ServiceRegistryConfig.INSTANCE.isIgnoreSwaggerDifference()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put if (ServiceRegistryConfig.INSTANCE.isIgnoreSwaggerDifference()) {
outside of
if (!StringUtils.isEmpty(scSchemaContent) && !StringUtils.isEmpty(localSchemaContent)) {
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. I have put it behind EQUALS block.
…ialization using swagger to parse different JDK versions.
Follow this checklist to help us incorporate your contribution quickly and easily:
[SCB-XXX] Fixes bug in ApproximateQuantiles
, where you replaceSCB-XXX
with the appropriate JIRA issue.mvn clean install -Pit
to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.