✨ Enhance local-dev for VirtualWorkspaces #3199
Conversation
kcp-ci-bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
kcp-ci-bot
added
size/L
mjudeikis
force-pushed
the
mjudeikis/vw.dev
branch
from
3cad42b to
26b8a6c
Compare
| // for the mini-front-proxy to use. The file should be in the format of the | ||
| // --mapping-file flag of the front-proxy. Do NOT expose this flag to users via main server options. | ||
| // It is overridden by the kcp start command. | ||
| AdditionalMappingsFile string |
There was a problem hiding this comment.
is this also written in the flag description? Probably should?
|
Direction looks fine. @embik do you have time for a detailed review? |
embik
left a comment
embik
left a comment
There was a problem hiding this comment.
Looks good to me overall, I'm just confused by the flag.
cmd/kcp/kcp.go
Outdated
| } else if f == "--mapping-file" { | ||
| if i < len(os.Args)-1 { | ||
| additionalMappingsFile = os.Args[i+1] | ||
| } // else let normal flag processing fail | ||
| } else if strings.HasPrefix(f, "--mapping-file") { | ||
| additionalMappingsFile = strings.TrimPrefix(f, "--mapping-file=") |
There was a problem hiding this comment.
I'm not sure I get the way this flag is set up. There is this code here in cmd/kcp/kcp.go, then there is another flag with the same name in cmd/kcp/options/generic.go (so this will be read into both serverOptions.Server.Extra.AdditionalMappingsFile and serverOptions.Server.GenericControlPlane.Extra.MappingFile as far as I can say). Then there is a comment on AdditionalMappingsFile in pkg/server/options/opttions.go saying it shouldn't be exposed.
Can you explain a little bit more here, maybe with comments on the code? Why is this early initialisation necessary, what is the relationship to the MappingFile value, etc? Do we really need both things?
There was a problem hiding this comment.
One is for front-proxy in production where we use this.
Another is at the server level for cases when we don't have a proxy. In this case when we run slim mini proxy with kcp start and we need to pass the flag in somehow.
maybe we can rename one at the server level to something dev-mapping-file so it's not confusing?
There was a problem hiding this comment.
But this PR doesn't touch the option in pkg/proxy/options/options.go, which is a separate implementation of --mapping-file. I think a --miniproxy-mapping-file flag on the server would be nicer, but I'm still not sure I understand why we read the flag in two places 😅
There was a problem hiding this comment.
Because we need to load it into fake-frontproxy :) So if we rename it will be cleaner - one for frontproxy, another for minifrontproxy.
There was a problem hiding this comment.
So TL:DR:
its 2 different kube apiservers - proxy and core kcp server. Core server should never have this flag defined. But because we share code with kcp start I didn't find nice way not to add it. Not its implied its development only.
kcp-ci-bot
added
the
needs-rebase
mjudeikis
force-pushed
the
mjudeikis/vw.dev
branch
from
95d91aa to
35f051d
Compare
kcp-ci-bot
removed
the
needs-rebase
|
LGTM label has been added. Git tree hash: 68168a7bc0e5ea4edda57b776f8a02cc260dfe22
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: embik The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kcp-ci-bot
added
the
approved
|
/kind feature |
kcp-ci-bot
added
the
kind/feature
4 participants
Summary
In this PR:
pkg/proxyintopkg/server/proxyso it could be reused inminiproxy. This is to avoid circular imports problem :/ didn't find better place for this.--mapping-fileforkcp startin dev mode, so once can start kcp locally and iterate on external virtual workspace binary code without having to deal with embedding code. It makes kcp core more standalone and forces people to think "core vs non-core"All in all, this makes local development of anything outside kcp (like mounts proxy) exponentially easier.
Related issue(s)
Fixes #
Release Notes