Skip to content

Conversation

@jhorvath
Copy link
Member

@jhorvath jhorvath commented Dec 3, 2024

Adding port forward command to the Oracle Cloud Assets

@jhorvath jhorvath added LSP [ci] enable Language Server Protocol tests VSCode Extension enterprise [ci] enable enterprise job labels Dec 3, 2024
@jhorvath jhorvath added this to the NB25 milestone Dec 3, 2024
@jhorvath jhorvath requested a review from sdedic December 3, 2024 14:05
@jhorvath jhorvath self-assigned this Dec 3, 2024
@neilcsmith-net
Copy link
Member

neilcsmith-net commented Dec 11, 2024

-1 to further changes here pending resolution of concerns in #7826 (comment) as it might conflict.

cc/ @matthiasblaesing

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 neilcsmith-net added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Dec 11, 2024
@MartinBalin
Copy link
Contributor

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???

@mbien
Copy link
Member

mbien commented Jan 8, 2025

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.

@neilcsmith-net
Copy link
Member

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.

@mbien
Copy link
Member

mbien commented Jan 8, 2025

So it happens

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.

@matthiasblaesing
Copy link
Contributor

@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 MartinBalin removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jan 15, 2025
@MartinBalin
Copy link
Contributor

I've removed do not merge label as discussion in #7826 concluded. Right?

@matthiasblaesing
Copy link
Contributor

I would have expected for #8162 to happen first, but yeah.

@MartinBalin
Copy link
Contributor

Matthias, I leave it up to you and Jan. Looks like I don't have full picture.

@matthiasblaesing
Copy link
Contributor

As PR #8162 is in I'll stop intervening.

@ebarboni ebarboni modified the milestones: NB25, NB26 Jan 24, 2025
@jhorvath jhorvath force-pushed the oci-port-forward branch 3 times, most recently from 6a374b3 to fcc4597 Compare March 10, 2025 11:02
@apache apache locked and limited conversation to collaborators Mar 10, 2025
@apache apache unlocked this conversation Mar 10, 2025
@jhorvath jhorvath requested a review from sdedic March 12, 2025 10:52
@jhorvath jhorvath merged commit 2a3dc1c into apache:master Mar 14, 2025
32 checks passed
@jhorvath jhorvath deleted the oci-port-forward branch March 14, 2025 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enterprise [ci] enable enterprise job LSP [ci] enable Language Server Protocol tests VSCode Extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants