You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "claudio4j (via GitHub)" <gi...@apache.org> on 2024/01/31 17:51:27 UTC
[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
<!-- Description -->
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)
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
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: `<group>:<artifact>:<type>:<version>:<classifier>`
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 `<group>:<artifact>:<type>` or `<group>:<artifact>:<classifier>`, 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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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, "<prop2>baz</prop2>")
assert.Contains(t, result, "<v2>bar</v2>")
}
+
+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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enhancement add classifier field to the MavenArtifact struct [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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:
```
<groupId>:<artifactId>:<packagingType>:<classifier>:<version>
<groupId>:<artifactId>:<packagingType>:<classifier>
<groupId>:<artifactId>:<packagingType>:<version>
<groupId>:<artifactId>:<packagingType>
<groupId>:<artifactId>:<classifier>:<version>
<groupId>:<artifactId>:<classifier>
<groupId>:<artifactId>:<version>
<groupId>:<artifactId>
```
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org