runtime: ensure that the generated project uses the right camel version #413
Conversation
lburgazzoli
force-pushed
the
github-384
branch
from
9312d05
to
5caca8f
Compare
lburgazzoli
force-pushed
the
github-384
branch
4 times, most recently
from
ee159f7
to
76c26e8
Compare
|
@astefanutti do you mind doing a review ? The list of impacted files is huge but that's because now the camel catalog is not more "global" but a function has been introduced to retrieve the one that matches the configured camel version (it always falls back to the default one if no matching catalog is found). There is something still left I'll address in future PRs |
pkg/builder/builder_steps.go
Outdated
| for _, d := range ctx.Project.Dependencies[:] { | ||
| if a, ok := ctx.Request.Catalog.Artifacts[d.ArtifactID]; ok { | ||
| for _, item := range a.Dependencies { | ||
| if item.GroupID == "" || item.ArtifactID == "" { |
There was a problem hiding this comment.
Is that expected to have a dependency without a groupId and an artifactId?
pkg/builder/builder_steps.go
Outdated
| continue | ||
| } | ||
| for _, item := range *a.Exclusions { | ||
| if item.GroupID == "" || item.ArtifactID == "" { |
There was a problem hiding this comment.
Is that expected to have an exclusion without a groupId and an artifactId?
There was a problem hiding this comment.
mh, I think it is a leftover for the testing phase, let me remove it
pkg/builder/builder_steps.go
Outdated
| t.Exclusions = &exclusions | ||
| } | ||
|
|
||
| *t.Exclusions = append(*t.Exclusions, item) |
There was a problem hiding this comment.
Would be possible that the exclusion is already there and shouldn't be duplicated?
pkg/builder/builder_steps.go
Outdated
| // | ||
| // post process dependencies | ||
| // | ||
| for _, d := range ctx.Project.Dependencies[:] { |
pkg/builder/builder_steps.go
Outdated
| // | ||
| // Add dependencies from catalog | ||
| // | ||
| for _, d := range ctx.Project.Dependencies[:] { |
There was a problem hiding this comment.
Not sure [:] is necessary? I need to review my slices-fu so that may be a trick to work with adding items while iterating!
There was a problem hiding this comment.
yes [:] is because of the need to modify the underlying thing, I may be wrong too :)
pkg/builder/builder_steps.go
Outdated
| continue | ||
| } | ||
|
|
||
| ctx.Project.AddDependency(item) |
There was a problem hiding this comment.
Are we sure it's correct to add dependencies while iterating on it?
pkg/builder/springboot/generator.go
Outdated
| @@ -149,5 +145,46 @@ func GenerateProject(ctx *builder.Context) error { | |||
| } | |||
| } | |||
|
|
|||
| // | |||
There was a problem hiding this comment.
Wonder if that could be factored with the default builder?
pkg/util/camel/catalog.go
Outdated
|
|
||
| catalogs = make(map[string]RuntimeCatalog) | ||
| } | ||
|
|
||
| // Runtime -- |
cmd/util/publisher/publisher.go
Outdated
| @@ -77,14 +78,15 @@ func (options *PublisherOptions) run(cmd *cobra.Command, args []string) { | |||
|
|
|||
| started := options.StartWith == "" | |||
|
|
|||
| keys := make([]string, 0, len(camel.Runtime.Artifacts)) | |||
| for k := range camel.Runtime.Artifacts { | |||
| catalog := camel.Runtime(defaults.CamelVersion) | |||
There was a problem hiding this comment.
Shouldn't be p.Spec.Build.CamelVersion? I imagine it's the same physically, still may be better logically...
pkg/util/camel/catalog.go
Outdated
| } | ||
|
|
||
| // Runtime -- | ||
| func Runtime(camelVersion string) *RuntimeCatalog { |
There was a problem hiding this comment.
I wonder whether the logic should be reversed. From an end-user standpoint, being able to specify a version in the platform spec following semver, and best matching the corresponding catalog, instead of matching x.y.z version with catalog x.y or x. I would find it more logical, but just a thought :)
There was a problem hiding this comment.
yes it make sense but I think it depends to the availability of different catalogs that we do not build by default now, do you mind rising an issue ?
lburgazzoli
force-pushed
the
github-384
branch
from
76c26e8
to
9ca2dad
Compare
|
@astefanutti I've fixed your findings, the only letf over is the latest comment about semver & co which I'm gogin to address in a separate pr while working on #302 |
pkg/util/camel/catalog.go
Outdated
| // Runtime -- | ||
| var Runtime Catalog | ||
| func Runtime(camelVersion string) *RuntimeCatalog { |
There was a problem hiding this comment.
I wonder whether we should rename the method to something like RuntimeCatalog or Catalog?
There was a problem hiding this comment.
renamed to func Catalog(...) *RuntimeCatalog
|
@lburgazzoli LGTM, just a nit-pick about the |
lburgazzoli
force-pushed
the
github-384
branch
from
9ca2dad
to
5502151
Compare
|
done |
Fixes #384
This is an interim solution until the runtime bits will be moved to Apache Camel as one of the supported platforms.
So far, the implementation works properly for integration running with Camel Main and not so well for Spring Boot ones as in such case transitive dependencies could introduce conflict among spring-boot versions.
What's left: