Skip to content

feat: implement config cache-proxy command#54

Merged
tisutisu merged 5 commits intokonflux-ci:mainfrom
tisutisu:implement-init-cmd
Feb 24, 2026
Merged

feat: implement config cache-proxy command#54
tisutisu merged 5 commits intokonflux-ci:mainfrom
tisutisu:implement-init-cmd

Conversation

@tisutisu
Copy link
Copy Markdown
Contributor

@tisutisu tisutisu commented Feb 9, 2026

This PR implements the konflux-build-cli config cache-proxy --enable <true/false> cmd, to be used during build-definitions init task replacement using golang CLI.

@tisutisu tisutisu requested a review from a team as a code owner February 9, 2026 05:52
@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented Feb 9, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Comment thread pkg/commands/cache_proxy.go Outdated
Comment on lines +89 to +97
l.Logger.Infof("cluster-config config map data: %v", cm.Data)
allowCache = cm.Data["allow-cache-proxy"]
if allowCache == "true" {
httpProxy = cm.Data["http-proxy"]
noProxy = cm.Data["no-proxy"]
} else {
httpProxy = ""
noProxy = ""
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this piece of code in else branch?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise setting default*Proxy has no effect. They are reassigned by reading http-proxy and no-proxy from cm.Data.

Copy link
Copy Markdown
Contributor Author

@tisutisu tisutisu Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have felt this same while writing the logic, but kept the logic same as per the current bash implementation here

Copy link
Copy Markdown
Contributor

@tkdchen tkdchen Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The below if-else is relative to the link you mentioned.

https://github.com/konflux-ci/build-definitions/blob/a91adc4f10bf5dc684fe32b373fd718253945cd4/task/init/0.3/init.yaml#L44-L61 is relative to the commented lines I think. Code from line 44 to 61 has this logic:

  • if configmap is present, http_proxy and no_proxy are assigned according to the config data.
  • if configmap is not present, use the defaults.

Copy link
Copy Markdown
Contributor Author

@tisutisu tisutisu Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got your point now, thanks for catching it.
Addressed it on the latest commit 787fee7

Copy link
Copy Markdown
Contributor

@tkdchen tkdchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can also wrap kubectl (it's included in task-runner) command instead of calling kubenetes APIs, which is similar with other nested commands. Invoking kubectl could also help us having less effort on maintaining dependencies updates.

@tisutisu
Copy link
Copy Markdown
Contributor Author

Maybe we can also wrap kubectl (it's included in task-runner) command instead of calling kubenetes APIs, which is similar with other nested commands. Invoking kubectl could also help us having less effort on maintaining dependencies updates.

@tkdchen even if we use cli, for reading/unmarshalling of kubectl cmd outputs, we need to depend on the kubernetes pkgs like k8s.io/api/core/v1, otherwise we have to duplicate code in our own implementation for reading ConfigMaps, not sure if we should go with wrapper around kubectl. WDYT?

Comment thread cmd/config/cache_proxy.go Outdated
Comment thread cmd/config/cache_proxy.go Outdated
Comment thread pkg/clients/kubernetes_test.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Copy link
Copy Markdown
Member

@mmorhun mmorhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be out of scope of this PR, but it anyway blocks it.
The main problem is that we accessing k8s cluster directly from within CLI making it tightly bound and dependent on k8s environment, making it impossible to run without a cluster, say locally or in another than Tekton CI. (Obviously, there are workarounds, but we need a proper solution).

My suggestion is to create a abstraction layer, say ConfigReader that would detect the environment (easiest is to use env var) and then read k8s resources if run within k8s cluster or access config in different source (say ini or env file) otherwise.

Comment thread cmd/config/cache_proxy.go Outdated
Comment thread pkg/clients/kubernetes.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
@tisutisu tisutisu force-pushed the implement-init-cmd branch 2 times, most recently from eddf14f to 8d1f0fd Compare February 16, 2026 13:35
Copy link
Copy Markdown
Member

@mmorhun mmorhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need ConfigReader interface with a factory (NewConfigReader) that constructs required implementation (K8sConfigReader or EnvFileConfigReader) based on an env var (if not set, k8s is the default). Then config command(s) can use it.

Comment thread cmd/config/cache_proxy.go Outdated
Comment thread pkg/appconfig/config_map_reader.go Outdated
Comment thread pkg/appconfig/config_map_reader.go Outdated
@tisutisu tisutisu force-pushed the implement-init-cmd branch 3 times, most recently from fa990a8 to d53204e Compare February 17, 2026 06:36
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/config/config_map_reader.go Outdated
Comment thread pkg/config/config_map_reader.go Outdated
Comment thread pkg/commands/cache_proxy.go
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/commands/cache_proxy.go Outdated
Comment thread pkg/config/config_reader.go
Comment thread pkg/config/config_reader.go Outdated
Comment thread testdata/sample-platform-config.ini Outdated
@mmorhun mmorhun requested a review from a team February 24, 2026 09:50
@tisutisu tisutisu merged commit b9f6e23 into konflux-ci:main Feb 24, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants