Re: [PR] fix(#5292): Simplify run cmd for archs other than amd64 [camel-k]
squakez merged PR #5403: URL: https://github.com/apache/camel-k/pull/5403 -- 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] fix(#5292): Simplify run cmd for archs other than amd64 [camel-k]
tdiesler commented on PR #5403: URL: https://github.com/apache/camel-k/pull/5403#issuecomment-2100694018 Yes -- 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] fix(#5292): Simplify run cmd for archs other than amd64 [camel-k]
squakez commented on PR #5403: URL: https://github.com/apache/camel-k/pull/5403#issuecomment-2100190284 @tdiesler is this one ready to merge? -- 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] fix(#5292): Simplify run cmd for archs other than amd64 [camel-k]
tdiesler commented on PR #5403: URL: https://github.com/apache/camel-k/pull/5403#issuecomment-2098201889 > LGTM. I think you can remove the comments in the jib.go which result misleading. Changed to ... ``` // Build the integration for the arch configured in the IntegrationPlatform. // This can explicitly be configured with the `-t builder.platforms` trait. // Building the integration for multiarch is deferred until the next major version (e.g. 3.x) ``` -- 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] fix(#5292): Simplify run cmd for archs other than amd64 [camel-k]
squakez commented on code in PR #5403:
URL: https://github.com/apache/camel-k/pull/5403#discussion_r1592250011
##
pkg/cmd/install.go:
##
@@ -594,6 +595,14 @@ func (o *installCmdOptions) setupIntegrationPlatform(c
client.Client, namespace
}
}
}
+
+ if platform.Status.Build.BuildConfiguration.ImagePlatforms == nil {
Review Comment:
This must not happen at installation time but at operator runtime. This
installation would only cover CLI based installation (which is going to
disappear). Beside that, the execution of the `runtime.GOARCH` would be
evaluated on the CLI machine, not on the cluster machine. This logic belong to
https://github.com/apache/camel-k/blob/main/pkg/platform/defaults.go
--
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] fix(#5292): Simplify run cmd for archs other than amd64 [camel-k]
tdiesler commented on PR #5403:
URL: https://github.com/apache/camel-k/pull/5403#issuecomment-2098050371
I now set `platform.Status.Build.BuildConfiguration.ImagePlatforms` like
[this](https://github.com/tdiesler/camel-k/blob/ghi5292/pkg/cmd/install.go#L599)
on `kamel install`
```
if platform.Status.Build.BuildConfiguration.ImagePlatforms == nil {
buildConfig :=
if runtime.GOARCH == "arm64" {
buildConfig.ImagePlatforms = []string{"linux/arm64"}
}
}
```
Later in jib.go, I try to read out that parameter like
[this](https://github.com/tdiesler/camel-k/blob/ghi5292/pkg/builder/jib.go#L116)
```
// If not explicitly configured otherwise, we build the integration for
the arch configured in the IntegrationPlatform.
// Building the integration for multiarch is deferred until the next
major version (e.g. 3.x)
var imagePlatforms []string
if t.task.Configuration.ImagePlatforms != nil {
imagePlatforms = t.task.Configuration.ImagePlatforms
} else {
if ip, _ := platform.GetForName(ctx, t.c, t.build.Namespace,
platform.DefaultPlatformName); ip != nil {
buildConfig := ip.Status.Build.BuildConfiguration
imagePlatforms = buildConfig.ImagePlatforms
log.Infof(">>>")
log.Infof(">>> IntegrationPlatform: %v", ip)
log.Infof(">>>")
log.Infof(">>> BuildConfiguration: %v", buildConfig)
log.Infof(">>>")
log.Infof(">>> ImagePlatforms: %v", imagePlatforms)
}
}
```
The expected `platform.Status.Build.BuildConfiguration.ImagePlatforms` is
always empty. I verified that this property is part of deep copy. What else
could be missing or needs checking?
--
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] fix(#5292): Simplify run cmd for archs other than amd64 [camel-k]
squakez commented on PR #5403: URL: https://github.com/apache/camel-k/pull/5403#issuecomment-2095357901 You don't need to create any new parameter. I think you can use IntegrationPlatform trait [1] setting and just add the default builder trait setting when it's missing. BTW, in order to be fully backward compatible, I think we must leave the setting empty when the default (amd64) is selected. You may need to `make generate` in order to update the CRDs accordingly. -- 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] fix(#5292): Simplify run cmd for archs other than amd64 [camel-k]
tdiesler commented on PR #5403:
URL: https://github.com/apache/camel-k/pull/5403#issuecomment-2093176168
I've created the property
[here](https://github.com/apache/camel-k/pull/5403/files#diff-eda3c0e8e19488b1353c8fe3c45149b065e9341263eff948eee48d5bac7590c7R138)
and initialized the default
[here](https://github.com/apache/camel-k/pull/5403/files#diff-48df85e2f487b8dbb6410a345e8b424a021dbaf817f24c3da1d6292dbaa8859eR359).
Then there is some deep copy magic
[here](https://github.com/apache/camel-k/pull/5403/files#diff-278beb1c64ee3eebc589b133c89f44799260ed5ed7b12a97a916a6c0e2d8c2cdR1461)
which I'm not supposed to edit
On introspection of the CRD, I don't see that property
```
{"level":"info","ts":"2024-05-03T14:40:20Z","logger":"camel-k","msg":">>>
IntegrationPlatform: &{{IntegrationPlatform camel.apache.org/v1} {camel-k
default 49f88784-8ed4-404f-b2f9-eb58859a5c78 14520 1 2024-05-03 14:38:36 +
UTC map[app:camel-k] map[camel.apache.org/operator.id:camel-k] []
[] [{kamel Update camel.apache.org/v1 2024-05-03 14:38:36 + UTC FieldsV1
{\"f:metadata\":{\"f:annotations\":{\".\":{},\"f:camel.apache.org/operator.id\":{}},\"f:labels\":{\".\":{},\"f:app\":{}}},\"f:spec\":{\".\":{},\"f:build\":{\".\":{},\"f:buildConfiguration\":{},\"f:maven\":{\".\":{},\"f:settings\":{},\"f:settingsSecurity\":{}},\"f:publishStrategy\":{},\"f:registry\":{\".\":{},\"f:address\":{},\"f:insecure\":{}}},\"f:kamelet\":{},\"f:traits\":{}}}
} {kamel Update camel.apache.org/v1 2024-05-03 14:38:40 + UTC FieldsV1
{\"f:status\":{\".\":{},\"f:build\":{\".\":{},\"f:baseImage\":{},\"f:buildConfiguration\":{\".\":{},\"f:orderStrategy\":{},\"f:strategy\":{}},\"f:maven\":
{\".\":{},\"f:cliOptions\":{},\"f:localRepository\":{}},\"f:maxRunningBuilds\":{},\"f:publishStrategy\":{},\"f:registry\":{\".\":{},\"f:address\":{},\"f:insecure\":{}},\"f:runtimeVersion\":{},\"f:timeout\":{}},\"f:cluster\":{},\"f:conditions\":{},\"f:info\":{\".\":{},\"f:gitCommit\":{},\"f:goOS\":{},\"f:goVersion\":{}},\"f:kamelet\":{\".\":{},\"f:repositories\":{}},\"f:observedGeneration\":{},\"f:phase\":{},\"f:version\":{}}}
status}]} { {{map[] map[] []} Jib{true 10.96.98.2 } nil nil {
map[] [] {nil nil} {nil nil} [] [] []} map[] 0 } {
map[] } [] {[]}} {{Kubernetes
{{ routine sequential map[] map[] []} Jib 3.8.1 eclipse-temurin:17 {true
10.96.98.2 } nil {Duration:5m0s,} {/etc/maven/m2 map[] [] {nil nil}
{nil nil} [] [] [-
V --no-transfer-progress -Dstyle.color=never]} map[] 3 } {
map[] } []
{[{none}]}} 1 Ready [{Created True 2024-05-03 14:38:40 + UTC 2024-05-03
14:38:40 + UTC IntegrationPlatformCreated integration platform created}
{RegistryAvailable True 2024-05-03 14:38:40 + UTC 2024-05-03 14:38:40 +
UTC IntegrationPlatformRegistryAvailable registry available at 10.96.98.2}
{CamelCatalogAvailable True 2024-05-03 14:38:40 + UTC 2024-05-03 14:38:40
+ UTC IntegrationPlatformCamelCatalogAvailable camel catalog 3.8.1
available}] 2.4.0-SNAPSHOT
map[gitCommit:60dcf5302ab354d6e800dcc5a73d69b6c00202ff goOS:linux
goVersion:go1.22.1]}}"}
{"level":"info","ts":"2024-05-03T14:40:20Z","logger":"camel-k","msg":">>>
ImagePlatform: "}
```
as a result, the Jib config does not see the imagePlatform default value.
Something is missing?
--
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] fix(#5292): Simplify run cmd for archs other than amd64 [camel-k]
tdiesler commented on PR #5403: URL: https://github.com/apache/camel-k/pull/5403#issuecomment-2076675654 Ready to merge? -- 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] fix(#5292): Simplify run cmd for archs other than amd64 [camel-k]
tdiesler commented on PR #5403:
URL: https://github.com/apache/camel-k/pull/5403#issuecomment-2074063570
The idea is, that (if not explicitly configured otherwise) we build the
integration for the arch we run on. Building the integration for multiarch is a
different story (and breaking change), which we deferred until the next major
version.
A configuration variable which is set at operator install time would likely
not add relevant information to what we get from the Go runtime already, or
would it?
`if runtime.GOARCH == "arm64"` is just a variation of a `switch` that I head
earlier. When more archs become relevant we can expand the mapping. Currently
it is ...
```
switch runtime.GOARCH {
arm64: linux/arm64
default: linux/amd64
}
```
--
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
