Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
claudio4j commented on PR #5126: URL: https://github.com/apache/camel-k/pull/5126#issuecomment-1931640559 Originally the `classifier` was before the `version` but that was wrong, as the `classifier` must be after `version`. Also, I reworked the ParseGAV function to accommodate the full GAV + classifier and it was a headache to change the regex, then I found it easier to use the `split` function and pick the elements from the array. Thanks for reviewing. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
squakez merged PR #5126: URL: https://github.com/apache/camel-k/pull/5126 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
github-actions[bot] commented on PR #5126: URL: https://github.com/apache/camel-k/pull/5126#issuecomment-1931115636 :heavy_check_mark: Unit test coverage report - coverage increased from 35.6% to 35.7% (**+0.1%**) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
squakez commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1477800792
##
pkg/apis/camel/v1/maven_types_support_test.go:
##
@@ -140,3 +140,51 @@ func TestMarshalPluginPropertiesWithNestedProps(t
*testing.T) {
assert.Contains(t, result, "baz")
assert.Contains(t, result, "bar")
}
+
+func TestArtifactToString(t *testing.T) {
+ a1 := MavenArtifact{
+ GroupID:"org.mygroup",
+ ArtifactID: "my-artifact",
+ }
+ assert.Equal(t, "mvn:org.mygroup:my-artifact", a1.GetDependencyID())
+
+ a2 := MavenArtifact{
+ GroupID:"org.mygroup",
+ ArtifactID: "my-artifact",
+ Type: "jar",
+ Version:"1.2",
+ Classifier: "foo",
+ }
+ assert.Equal(t, "mvn:org.mygroup:my-artifact:jar:foo:1.2",
a2.GetDependencyID())
+
+ a3 := MavenArtifact{
+ GroupID:"org.mygroup",
+ ArtifactID: "my-artifact",
+ Version:"1.2",
+ }
+ assert.Equal(t, "mvn:org.mygroup:my-artifact:1.2", a3.GetDependencyID())
+
+ a4 := MavenArtifact{
+ GroupID:"org.mygroup",
+ ArtifactID: "my-artifact",
+ Type: "jar",
+ Classifier: "foo",
+ }
+ assert.Equal(t, "mvn:org.mygroup:my-artifact:jar:foo",
a4.GetDependencyID())
+
+ a5 := MavenArtifact{
+ GroupID:"org.mygroup",
+ ArtifactID: "my-artifact",
+ Classifier: "foo",
+ Version:"1.2",
+ }
+ assert.Equal(t, "mvn:org.mygroup:my-artifact::foo:1.2",
a5.GetDependencyID())
Review Comment:
the `::` looks suspicious. Are they really expected that way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
claudio4j commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1476701427
##
pkg/apis/camel/v1/maven_types_support.go:
##
@@ -24,7 +24,11 @@ import (
func (in *MavenArtifact) GetDependencyID() string {
switch {
case in.Version == "":
- return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ if in.Classifier == "" {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ } else {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID + ":"
+ in.Type + ":" + in.Classifier
Review Comment:
I clarified the behavior and added more testing.
##
pkg/apis/camel/v1/maven_types_support.go:
##
@@ -24,7 +24,11 @@ import (
func (in *MavenArtifact) GetDependencyID() string {
switch {
case in.Version == "":
- return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ if in.Classifier == "" {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ } else {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID + ":"
+ in.Type + ":" + in.Classifier
Review Comment:
I clarified the behavior and added more tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
claudio4j commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1476701427
##
pkg/apis/camel/v1/maven_types_support.go:
##
@@ -24,7 +24,11 @@ import (
func (in *MavenArtifact) GetDependencyID() string {
switch {
case in.Version == "":
- return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ if in.Classifier == "" {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ } else {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID + ":"
+ in.Type + ":" + in.Classifier
Review Comment:
I changed the behavior and added more testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
squakez commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1476009977
##
pkg/apis/camel/v1/maven_types_support.go:
##
@@ -24,7 +24,11 @@ import (
func (in *MavenArtifact) GetDependencyID() string {
switch {
case in.Version == "":
- return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ if in.Classifier == "" {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ } else {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID + ":"
+ in.Type + ":" + in.Classifier
Review Comment:
Then we need to enforce a check to verify that both classifier and type are
present. Something like: `if in.Classifier != "" && in.Type != ""` in order to
avoid to have a possible runtime value such as "mvn:gid:art::classif". We may
even see if an error return value makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
claudio4j commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1476002653
##
pkg/apis/camel/v1/maven_types_support.go:
##
@@ -24,7 +24,11 @@ import (
func (in *MavenArtifact) GetDependencyID() string {
switch {
case in.Version == "":
- return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ if in.Classifier == "" {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ } else {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID + ":"
+ in.Type + ":" + in.Classifier
Review Comment:
The definition of the full dependency is:
``
However the dependency regex and the other checkers were not handling the
`type` field, and I didn't want to spend too much time finding the right regex
to handle the type or the classifier in isolation, because doing so would
require to change the dependency spec to allow either
`::` or `::`, but the
`type` and `classifier` may be any word, so we would have to determine a way
to set both cases.
Then, the solution I found at this moment is if the `classifier` is set,
then the `type` is a required field.
I suggest to open a new issue to better investigate this particular issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
squakez commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1475979381
##
pkg/apis/camel/v1/maven_types_support.go:
##
@@ -24,7 +24,11 @@ import (
func (in *MavenArtifact) GetDependencyID() string {
switch {
case in.Version == "":
- return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ if in.Classifier == "" {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID
+ } else {
+ return "mvn:" + in.GroupID + ":" + in.ArtifactID + ":"
+ in.Type + ":" + in.Classifier
Review Comment:
Maybe we should check the presence of the type as well, is it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
squakez commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1474843154
##
pkg/util/maven/maven_project_test.go:
##
@@ -181,6 +181,17 @@ func TestParseGAVMvnNoVersion(t *testing.T) {
assert.Equal(t, dep.ArtifactID, "camel-core")
}
+func TestParseGAVMvnClassifierNoVersion(t *testing.T) {
+ dep, err := ParseGAV("org.apache.camel:camel-k-core:jar:custom")
Review Comment:
No, we're missing all the permutations, which are all the possible
combinations with and without the fields:
```
:::
:::
::
:::
::
::
:
```
I did the math wrong, they are finally 8. As we're introducing more
elements, we better have coverage of all possible scenarios. On the contrary we
may be in the bad situation of looking for a bug in during a production
workflow. I think it is worth the effort to test them all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
claudio4j commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1474830759
##
pkg/trait/jolokia.go:
##
@@ -124,7 +114,7 @@ func (t *jolokiaTrait) Apply(e *Environment) error {
jolokiaFilepath := ""
for _, ar := range e.IntegrationKit.Status.Artifacts {
- if strings.HasPrefix(ar.ID, "org.jolokia.jolokia-jvm") {
+ if strings.HasPrefix(ar.ID, "org.jolokia.jolokia-agent-jvm") {
Review Comment:
The old and new artifact are expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
claudio4j commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1474829787
##
pkg/util/camel/camel_dependencies.go:
##
@@ -298,6 +298,7 @@ func addDependenciesFromCatalog(project *maven.Project,
catalog *RuntimeCatalog)
md := maven.Dependency{
GroupID:dep.GroupID,
ArtifactID: dep.ArtifactID,
+ Classifier: dep.Classifier,
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
claudio4j commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1474822411
##
pkg/util/maven/maven_project_test.go:
##
@@ -181,6 +181,17 @@ func TestParseGAVMvnNoVersion(t *testing.T) {
assert.Equal(t, dep.ArtifactID, "camel-core")
}
+func TestParseGAVMvnClassifierNoVersion(t *testing.T) {
+ dep, err := ParseGAV("org.apache.camel:camel-k-core:jar:custom")
Review Comment:
The only missing test case is group:artifact:classifier (no type, no
version), as the current regex grouping does some assumption, which don't
support the mentioned test case and I think it's not worth to spend much time
to change this regex to accommodate this particular use case.
We can add a requirement, if there is a classifier, then a type is also
required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
squakez commented on code in PR #5126:
URL: https://github.com/apache/camel-k/pull/5126#discussion_r1474033347
##
pkg/util/maven/maven_project_test.go:
##
@@ -181,6 +181,17 @@ func TestParseGAVMvnNoVersion(t *testing.T) {
assert.Equal(t, dep.ArtifactID, "camel-core")
}
+func TestParseGAVMvnClassifierNoVersion(t *testing.T) {
+ dep, err := ParseGAV("org.apache.camel:camel-k-core:jar:custom")
Review Comment:
I'd do a permutation of the possible cases to make sure we're not missing
something. We should have at least 7 different cases to test.
##
pkg/apis/camel/v1/maven_types.go:
##
@@ -81,14 +81,18 @@ type RepositoryPolicy struct {
ChecksumPolicy string `xml:"checksumPolicy,omitempty"
json:"checksumPolicy,omitempty"`
}
-// MavenArtifact defines a GAV (Group:Artifact:Version) Maven artifact.
+// MavenArtifact defines a GAV (Group:Artifact:Classifier:Version) Maven
artifact.
Review Comment:
Should we have the `type` as well here?
##
pkg/trait/jolokia.go:
##
@@ -124,7 +114,7 @@ func (t *jolokiaTrait) Apply(e *Environment) error {
jolokiaFilepath := ""
for _, ar := range e.IntegrationKit.Status.Artifacts {
- if strings.HasPrefix(ar.ID, "org.jolokia.jolokia-jvm") {
+ if strings.HasPrefix(ar.ID, "org.jolokia.jolokia-agent-jvm") {
Review Comment:
We should not change this until the new catalog is ready. Let's make it
compatible with the catalog we are using and we'll change to the new values
once we have the new catalog.
##
pkg/apis/camel/v1/maven_types_support.go:
##
@@ -19,12 +19,18 @@ package v1
import (
"encoding/xml"
+ "fmt"
)
func (in *MavenArtifact) GetDependencyID() string {
+ fmt.Printf(">>> maven_types_support GetDependencyID: +%+v\n", in)
Review Comment:
Debug line.
##
pkg/util/camel/camel_dependencies.go:
##
@@ -298,6 +298,7 @@ func addDependenciesFromCatalog(project *maven.Project,
catalog *RuntimeCatalog)
md := maven.Dependency{
GroupID:dep.GroupID,
ArtifactID: dep.ArtifactID,
+ Classifier: dep.Classifier,
Review Comment:
If we're having the `type` parameter, I think it makes sense to include it
here as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
claudio4j opened a new pull request, #5126: URL: https://github.com/apache/camel-k/pull/5126 The classifier is relevant, since jolokia 2.0.0 jvm agent is resolved by using a [GAV classifier](https://jolokia.org/reference/html/manual/agents.html#jvm-agent). The change in camel-k-catalog is in this [PR #1161](https://github.com/apache/camel-k-runtime/pull/1161) **Release Note** ```release-note feature: add classifier field to the maven artifact structure ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
