Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ Experimental mounts API #3057

Merged
merged 2 commits into from
Dec 25, 2023
Merged

✨ Experimental mounts API #3057

merged 2 commits into from
Dec 25, 2023

Conversation

mjudeikis
Copy link
Contributor

@mjudeikis mjudeikis commented Dec 9, 2023 •
edited
Loading

Summary

Adds experimental mounts API to be able to mount workspaces under the hood to different destinations.

Related issue(s)

Fixes #

Release Notes

Add experimental mounts API

@kcp-ci-bot kcp-ci-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the DCO. labels Dec 9, 2023
@kcp-ci-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kcp-ci-bot kcp-ci-bot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 9, 2023
@mjudeikis mjudeikis mentioned this pull request Dec 9, 2023
Merged
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.api.proposal branch from 212898a to 4dcdb6b Compare December 13, 2023 18:48
@kcp-ci-bot kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2023
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.api.proposal branch from 4dcdb6b to b9142d9 Compare December 13, 2023 18:53
@kcp-ci-bot kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2023
@mjudeikis mjudeikis marked this pull request as ready for review December 13, 2023 18:54
@kcp-ci-bot kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2023
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.api.proposal branch from b9142d9 to 951fe01 Compare December 13, 2023 19:08
@mjudeikis mjudeikis requested a review from sttts December 13, 2023 19:12
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.api.proposal branch from 951fe01 to 4f62803 Compare December 14, 2023 16:40
embik
embik reviewed Dec 19, 2023
View reviewed changes
Copy link
Member

@embik embik left a comment

Choose a reason for hiding this comment

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

Some minor Qs.

pkg/index/index.go Outdated Show resolved Hide resolved
pkg/server/localproxy.go
}
logger.WithValues("from", path, "to", r.URL).V(4).Info("mounting cluster")
proxy := httputil.NewSingleHostReverseProxy(url)
// TODO(mjudeikis): remove this once we have a real cert wired in dev mode
Copy link
Member

Choose a reason for hiding this comment

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

Where would this cert come from / what is missing to add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be kcp start generated certificate for the local run. It is just tedious wiring in for trust. Missing - bit more time, but want this to settle down before doing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would todo this once I have more building blocks merged and know these things do not move. If this ok, going to do it as a follow-up PR.

I created this and assigned myself: #3065

I am still a bit hazy in my head about how this would work when the actor develops external VirtualWorkspaces.
I was thinking todo some example-virtual-workspace repo/code when all this materialized and at that point certificate should be clear :)

sttts
sttts reviewed Dec 22, 2023
View reviewed changes
pkg/index/index.go Outdated Show resolved Hide resolved
sttts
sttts reviewed Dec 22, 2023
View reviewed changes
pkg/index/index.go Outdated Show resolved Hide resolved
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.api.proposal branch 2 times, most recently from 5400de3 to 02d1211 Compare December 22, 2023 15:53
@kcp-ci-bot kcp-ci-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. dco-signoff: no Indicates the PR's author has not signed the DCO. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. dco-signoff: yes Indicates the PR's author has signed the DCO. labels Dec 22, 2023
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.api.proposal branch from 02d1211 to 23a05fe Compare December 22, 2023 15:54
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.api.proposal branch from 23a05fe to b47f859 Compare December 22, 2023 15:55
@kcp-ci-bot kcp-ci-bot added dco-signoff: yes Indicates the PR's author has signed the DCO. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed dco-signoff: no Indicates the PR's author has not signed the DCO. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 22, 2023
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.api.proposal branch from b47f859 to 8d99070 Compare December 22, 2023 16:11
@sttts
Copy link
Member

sttts commented Dec 25, 2023

/lgtm
/approve

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 25, 2023
@kcp-ci-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a5715a81174e830bb4a0f068ce18c179e733cdee

@kcp-ci-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kcp-ci-bot kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 25, 2023
@kcp-ci-bot kcp-ci-bot merged commit 1a7b48e into kcp-dev:main Dec 25, 2023
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@embik embik embik left review comments

@sttts sttts sttts left review comments

Assignees

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mjudeikis @kcp-ci-bot @sttts @embik