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
KEP-29: Handle changed dependencies when upgrading operators #1558
Conversation
// Dependencies are resolved recursively. | ||
// Cyclic dependencies are detected and result in an error. | ||
func gatherDependencies(root packages.Resources, resolver pkgresolver.Resolver) ([]packages.Resources, error) { | ||
func Resolve(root packages.Resources, resolver pkgresolver.Resolver) ([]packages.Resources, error) { |
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'm not sure about this renaming. So far Resolve
methods in KUDOs codebase were implementations of the Resolver interface. But then again the name sort of fits 🤔
@@ -68,25 +70,27 @@ func Package( | |||
dependency.Operator.SetNamespace(namespace) | |||
dependency.OperatorVersion.SetNamespace(namespace) | |||
|
|||
if err := installOperatorAndOperatorVersion(client, dependency); err != nil { | |||
if err := install.OperatorAndOperatorVersion( |
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.
Are you coding in 80 characters wide terminal? ^.^
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.
120 chars. But having long single lines decreases readability, because one may need to scroll and then loses context of the surrounding lines.
When upgrading operators with dependencies, its dependencies could have changed. In this case the Operator and OperatorVersion resources of the new dependencies are installed. Outdated OperatorVersion resources aren't removed. The code was refactored to decouple dependency handling from package installation and have separate packages to install and upgrade KUDO resources. Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
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.
A few nits and let's clarify the installation of the dependencies with regard to the operator name. Otherwise, this looks solid.
Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Operator installation has been refactored to include dependency handling. Hence, upgrades are delegating dependency handling to O/OV installation. Signed-off-by: Jan Schlicht <jan@d2iq.com>
} | ||
|
||
if !funk.ContainsString(versionsInstalled, operatorVersion.Spec.Version) { | ||
if err := installDependencies(client, operatorVersion, resolver); err != nil { |
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.
Dependencies are now handled directly in install.OperatorAndOperatorVersion
. This function is called when installing packages (install.Package
) as well as upgrading OVs (upgrade.OperatorVersion
).
Signed-off-by: Jan Schlicht <jan@d2iq.com>
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.
🚢 👏
What this PR does / why we need it:
When upgrading operators with dependencies, its dependencies could have changed. In this case the Operator and OperatorVersion resources of the new dependencies are installed. Outdated OperatorVersion resources aren't removed.
The code was refactored to decouple dependency handling from package installation and have separate packages to install and upgrade KUDO resources.
Refactorings:
kudoctl/packages/install
tokudoctl/resources/dependencies
kudoctl/packages/install
tokudoctl/resources/install
kudoctl/util/kudo
tokudoctl/resources/upgrade
Fixes #1514