-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: Upgrade built-in graphs automatically (DEV-3552) #3216
Conversation
Remove the need to add a MigrateOnlyBuiltInGraphs to the RepositoryUpdatePlan
// Yes. Nothing more to do. | ||
ZIO.succeed(RepositoryUpdatedResponse(s"Repository is up to date at $requiredRepositoryVersion")) | ||
} else { | ||
if (repositoryVersion.contains(KnoraBaseVersion)) { |
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.
shouldn't it check that it's strictly smaller? or are you anyway relying on the fact that only the plugins that are bigger are filtered?
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.
I have not changed the general logic of this:
If the version in the db does not exactly match the version of our server we need do proceed with doing an update.
Are you referring to the edge case of having a db version which is greater than the server version? This case was and still is not covered. What do you think should happen? I guess it would be best to prevent the startup entirely in this case.
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.
Yes, I think it would make most sense to not do anything in case it's bigger.
(Note: comment edited.)
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.
I will add this behaviour in this PR then.
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.
Would this case be even possible?
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 happens regularly on dev servers, when the API version deployed there is lower than the one on prod, and then prod data gets mirrored to that dev server.
And previously, in this scenario the start-up would fail because of the exception raised in L139 which killed the fiber. I think we should keep that behaviour, unless we have good reasons to change it.
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 commit add the behaviour back again:
bf97820
webapi/src/main/scala/org/knora/webapi/store/triplestore/upgrade/RepositoryUpdater.scala
Outdated
Show resolved
Hide resolved
webapi/src/main/scala/org/knora/webapi/store/triplestore/upgrade/RepositoryUpdatePlan.scala
Show resolved
Hide resolved
// Yes. Nothing more to do. | ||
ZIO.succeed(RepositoryUpdatedResponse(s"Repository is up to date at $requiredRepositoryVersion")) | ||
} else { | ||
if (repositoryVersion.contains(KnoraBaseVersion)) { |
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.
Would this case be even possible?
Yes, every time the application is restarting. |
Replace if else with pattern matching Replace Option.get with 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.
Thanks!
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! There is still some test failing...
Seems to be a fluke. org.knora.webapi.it.v2.KnoraSipiIntegrationV2ITSpec pass locally and after a rerun. |
The last releases where updating only the built in graphs.
The built in graphs should be upgraded automatically added in certain cases.
An upgrade is necessary under these conditions:
When collecting the necessary plugins results in an empty list of plugins then a
PluginForKnoraBaseVersion(versionNumber = serverVersion, plugin = new MigrateOnlyBuiltInGraphs)
should be run.When collecting the necessary plugins to run results in a list which contains at least one plugin nothing has to be done because every UpgradePlugin must provide a MigrateSpecificGraphs object which in all cases contains the built in graphs.
Pull Request Checklist
Task Description/Number
Issue Number: DEV-3552
PR Type
Basic requirements for bug fixes and features
Does this PR introduce a breaking change?
Does this PR change client-test-data?