-
Notifications
You must be signed in to change notification settings - Fork 344
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
Context handling improvement #76
Conversation
2c1fd71
to
2150bce
Compare
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.
Sounds really good. It was a bit difficult to follow, due to the many return points in the "Handle" method, but the logic is correct.
Computation of the most similar context is useful only in case we start to chain builds, but now we create them from scratch. Using a context that has more dependencies than needed is error prone (think to maven version resolution: adding a new dependency changes the resolution mechanism and versions of unrelated libraries may change).
I'd stick with exact match for now. Wdyt?
func NewCmdInstall(rootCmdOptions *RootCmdOptions) *cobra.Command { | ||
options := InstallCmdOptions{ | ||
options := installCmdOptions{ |
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, I'll setup golint and add it to the developer guide :)
if err != nil { | ||
return err | ||
} | ||
|
||
err = install.PlatformContexts(namespace) |
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.
It's ok for now. Later with #73 it will be the operator to install those resources. Installation should be limited to the minimum set of dependencies (operator, crd and rbac).
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.
+1
pkg/stub/action/integration/util.go
Outdated
// The integration has no dependencies, try to find the one that | ||
// has minimum dependencies requirement. | ||
|
||
ndeps := math.MaxUint16 |
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.
The final rule should be:
- Take "X" if has the same set of dependencies
- If no context has the same set, then create a context from the most similar one (advanced) or just create a new one
- Don't use context "Y" if there is even just 1 library in "Y" that is not required by the integration (a.k.a. don't put unrelated garbage in the JVM)
Wdyt?
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.
Sounds good and simpler
pkg/stub/action/integration/util.go
Outdated
} | ||
|
||
if c == nil { | ||
// TODO: generate a new platform 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.
Don't you do it in the caller function?
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, leftover
pkg/stub/action/integration/build.go
Outdated
buildIdentifier := api.BuildIdentifier{ | ||
Name: integration.Name, | ||
Qualifier: integration.Status.Digest, | ||
platformCtxName := fmt.Sprintf("ctx-%s-%s", integration.Name, integration.ResourceVersion) |
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 think platform contexts don't need to be named after integrations. They can be shared among integrations and "must" be immutable, i.e. when a integration change dependencies, a new context should be created, leaving the old one to a "garbage collector". Wdyt?
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 this was for "tracking purpose" while building, we'll define an independent name.
2150bce
to
57203fc
Compare
Yeah, I did try to implement something smart but was unsure as it leave some unknowns. I've fixed your findings, please have a look |
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.
Perfect!
Fixes #75
Fixes #74