Adding port forward to OKE Pod #8014
Conversation
jhorvath
added
LSP
| .withName(name) | ||
| .get(); | ||
| if (deployment == null) { | ||
| continue; |
There was a problem hiding this comment.
no log / failed (ignored) result to the caller ?
| @@ -67,8 +64,8 @@ static void runWithClient(ClusterItem cluster, Consumer<KubernetesClient> consum | |||
| Config config = prepareConfig(cluster.getConfig(), cluster); | |||
| try (KubernetesClient client = new KubernetesClientBuilder().withConfig(config).build();) { | |||
| consumer.accept(client); | |||
| } catch (Exception e) { | |||
| Exceptions.printStackTrace(e); | |||
| } catch (Throwable t) { | |||
| */ | ||
| public void startPortForward(PodItem podItem) { | ||
| if (activePortForwards.containsKey(podItem.getName())) { | ||
| NotificationUtils.showMessage("Port forwarding already active for: " + podItem.getName()); |
| List<PortForwardItem> forwardItems = new ArrayList<>(); | ||
| List<PortForward> forwards = new ArrayList<>(); | ||
| if (ports.isEmpty()) { | ||
| NotificationUtils.showMessage("No ports found for pod: " + podItem.getName()); |
| Pod pod = client.pods().inNamespace(podItem.getNamespace()).withName(podItem.getName()).get(); | ||
|
|
||
| if (pod == null) { | ||
| System.out.println("Pod not found: " + podItem.getName()); |
There was a problem hiding this comment.
Log or NotificationUtils.showMessage ?
| NotificationUtils.showMessage(Bundle.ForwardingPorts(port.toString(), podItem.getName())); | ||
| } | ||
| latches.put(podItem.getName(), latch); | ||
| List<PortForwardItem> oldValue = activePortForwards.put(podItem.getName(), forwardItems); |
There was a problem hiding this comment.
The old value does not make much sense: even if old value is later restored, its latch is lost for good.
| activePortForwards.remove(podItem.getName()); | ||
| firePropertyChange(podItem, forwardItems, oldValue); | ||
| } catch (InterruptedException | IOException ex) { | ||
| Exceptions.printStackTrace(ex); |
There was a problem hiding this comment.
Shouldn't this error notification be presented to the user somehow ?
| @@ -188,8 +197,18 @@ private void runInCluster() { | |||
| .create(); | |||
| } | |||
| }); | |||
| } catch(Throwable t) { | |||
|
|
||
| InputOutputProvider<?, ?, ?, ?> newSpiDef | ||
| = Lookup.getDefault().lookup(InputOutputProvider.class); | ||
| Object io = newSpiDef.getIO("test io", true, Lookup.EMPTY); |
There was a problem hiding this comment.
This seems as an unfinished implementation ?
| @@ -87,5 +86,10 @@ public static ChildrenProvider.SessionAware<CompartmentItem, ClusterItem> getClu | |||
| .collect(Collectors.toList()); | |||
| }; | |||
| } | |||
|
|
|||
| @Override | |||
jhorvath
force-pushed
the
oci-port-forward
branch
from
d58dfb9 to
9d2e3c1
Compare
|
-1 to further changes here pending resolution of concerns in #7826 (comment) as it might conflict. Might have limited time for NetBeans in coming weeks, so adding a note that I'm happy for @matthiasblaesing or @mbien to remove this -1 and the do-not-merge label as and when they're happy with resolution. |
neilcsmith-net
added
the
do not merge
|
I don't understand the do-not-merge label here and why there is some discussion about it in old already merged and released PR??? |
@MartinBalin not sure what is confusing here. The PR in question was not discussed prior to integration and added >30 jars. The sig file had to be turned off since it would be 14MB of method signatures alone. This PR here appears to build on top of the other PR. Things have to be cleared up in the right order. there was 0 visible evaluation done, the PR text is one sentence. The first comment with some elaboration is two days old #7826 (comment) And yes, this absolutely should not have been discussed after integration - I agree with you there. |
Ideally, yes, but we are far more towards commit-then-review than review-then-commit. So it happens. Sometimes things need rolling back. Ideally the people discussing that would have applied the veto on this! 😉 Instead I've given permission to remove mine. I just want to see the concerns resolved before things build on top of them, and we end up with an even more complicated situation to manage. |
I am sorry, but one sentence to add 30 jars is not enough. This repo is treated like the wild west by some. I don't know any other project, closed or open, which would allow things like this. Lets try to open a PR against openjdk with one sentence which adds 30 dependencies. We just had another unsquashed PR merged today which added 15 commits - I am out of ideas tbh. |
|
@MartinBalin the discussion was in PR #7826, because the problem (wrong license, massive dependency set) was introduced there. The do-no-merge label was set here to stop further changes to the module and a fix for the license first, well, that did not work.... There are currently two show stoppers:
Not required now is fixing the size of the libraries. Either Oracles API is really such a mess or there needs to be better ways to bind a required subset of functions/structures for what we need. Extrapolate 40MB (or 26MB if I ignore kubernetes as "common") of dependencies to multiple providers and I think it is clear why I think, that needs some thoughts. |
MartinBalin
removed
the
do not merge
|
I've removed |
|
I would have expected for #8162 to happen first, but yeah. |
|
Matthias, I leave it up to you and Jan. Looks like I don't have full picture. |
|
As PR #8162 is in I'll stop intervening. |
Adding port forward command to the Oracle Cloud Assets