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.

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

Turn RPC State forwardedPorts into map keyed by the local port #2659

Merged
merged 2 commits into from
Aug 16, 2019

Conversation

briandealwis
Copy link
Member

Fixes #2657 by turning forwardedPorts into a map keyed by the local port. With this change, I see the full list of forwarded ports as I was expecting. Using the local port also ensures that reallocated ports are overwritten.

$ http localhost:50052/v1/state
HTTP/1.1 200 OK
Content-Length: 1329
Content-Type: application/json
Date: Thu, 15 Aug 2019 19:17:48 GMT
Grpc-Metadata-Content-Type: application/grpc

{
    "buildState": {
        "artifacts": {
            "gcr.io/k8s-skaffold/skaffold-debug-jib": "Complete",
            "gcr.io/k8s-skaffold/skaffold-debug-nodejs": "Complete",
            "gcr.io/k8s-skaffold/skaffold-debug-npm": "Complete",
            "gcr.io/k8s-skaffold/skaffold-debug-python3": "Complete"
        }
    },
    "deployState": {
        "status": "Complete"
    },
    "forwardedPorts": {
        "3000": {
            "containerName": "web",
            "localPort": 3000,
            "namespace": "default",
            "podName": "nodejs",
            "remotePort": 3000,
            "resourceName": "nodejs",
            "resourceType": "pod"
        },
        "5000": {
            "containerName": "web",
            "localPort": 5000,
            "namespace": "default",
            "podName": "python3",
            "remotePort": 5000,
            "resourceName": "python3",
            "resourceType": "pod"
        },
        "5005": {
            "containerName": "web",
            "localPort": 5005,
            "namespace": "default",
            "podName": "jib-6769b7f8d7-j6f8s",
            "portName": "jdwp",
            "remotePort": 5005,
            "resourceName": "jib-6769b7f8d7-j6f8s",
            "resourceType": "pod"
        },
        "5678": {
            "containerName": "web",
            "localPort": 5678,
            "namespace": "default",
            "podName": "python3",
            "portName": "dap",
            "remotePort": 5678,
            "resourceName": "python3",
            "resourceType": "pod"
        },
        "8080": {
            "containerName": "web",
            "localPort": 8080,
            "namespace": "default",
            "podName": "jib-6769b7f8d7-j6f8s",
            "remotePort": 8080,
            "resourceName": "jib-6769b7f8d7-j6f8s",
            "resourceType": "pod"
        },
        "9229": {
            "containerName": "web",
            "localPort": 9229,
            "namespace": "default",
            "podName": "nodejs",
            "portName": "devtools",
            "remotePort": 9229,
            "resourceName": "nodejs",
            "resourceType": "pod"
        }
    }
}

This PR uses a new field number, so it should be backwards compatible (though I'm not sure how about the JSON interface).

Neither Cloud Code for VSCode nor IntelliJ use the state object and so are unaffected.

(Note: you may notice that the npm container has no ports listed; they're somehow terminated. Will file that separately.)

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

this change seems fine, looks like you need to fix the tests though.

I'm also wondering if we should do some sort of versioning for the proto/API here. backwards compatibility shouldn't be an issue here but I'm not quite sure what the right approach is here.

@balopat balopat merged commit 66d482c into GoogleContainerTools:master Aug 16, 2019
@briandealwis briandealwis deleted the i2657 branch August 30, 2019 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPC State for port forwards assumes unique container names
4 participants