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

Fixed the retrieval of the local LSP4IJ path. #1247

Merged

Conversation

anusreelakshmi934
Copy link
Contributor

Fixes #1242

@anusreelakshmi934
Copy link
Contributor Author

anusreelakshmi934 commented Jan 29, 2025

Copy link
Contributor

@dessina-devasia dessina-devasia left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@dessina-devasia dessina-devasia self-requested a review January 29, 2025 12:54
Copy link
Contributor

@dessina-devasia dessina-devasia left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mrglavas mrglavas left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Contributor

@mrglavas mrglavas left a comment

Choose a reason for hiding this comment

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

@anusreelakshmi934 It occurred to me that there's an unzip step in our build: https://github.com/OpenLiberty/liberty-tools-intellij/blob/main/.github/workflows/build.yaml that seems no longer necessary with these changes. We should clean that up to avoid any confusion.

image

@anusreelakshmi934
Copy link
Contributor Author

anusreelakshmi934 commented Jan 30, 2025

@anusreelakshmi934 It occurred to me that there's an unzip step in our build: https://github.com/OpenLiberty/liberty-tools-intellij/blob/main/.github/workflows/build.yaml that seems no longer necessary with these changes. We should clean that up to avoid any confusion.

image

Yes @mrglavas . I have cleaned up the code.
Here is the Cron Job link after the change: link.

In this run, the 24.0.9 build fails because it lacks the unzip step, preventing it from retrieving the lsp4ij folder correctly. However, the 24.0.12 build succeeds since the condition is not functioning as expected.
Should we proceed with this change despite the failure in 24.0.9?

@mrglavas
Copy link
Contributor

mrglavas commented Jan 30, 2025

@anusreelakshmi934 It occurred to me that there's an unzip step in our build: https://github.com/OpenLiberty/liberty-tools-intellij/blob/main/.github/workflows/build.yaml that seems no longer necessary with these changes. We should clean that up to avoid any confusion.
image

Yes @mrglavas . I have cleaned up the code. Here is the Cron Job link after the change: link.

In this run, the 24.0.9 build fails because it lacks the unzip step, preventing it from retrieving the lsp4ij folder correctly. However, the 24.0.12 build succeeds since the condition is not functioning as expected. Should we proceed with this change despite the failure in 24.0.9?

@anusreelakshmi934 I believe in our team discussion earlier today we agreed that the unzip step should be restored to allow the 24.0.9 build to run. We also agreed that a comment be added to document that the unzip step is required for compatibility with the 24.0.9 build.

@anusreelakshmi934
Copy link
Contributor Author

@anusreelakshmi934 It occurred to me that there's an unzip step in our build: https://github.com/OpenLiberty/liberty-tools-intellij/blob/main/.github/workflows/build.yaml that seems no longer necessary with these changes. We should clean that up to avoid any confusion.
image

Yes @mrglavas . I have cleaned up the code. Here is the Cron Job link after the change: link.
In this run, the 24.0.9 build fails because it lacks the unzip step, preventing it from retrieving the lsp4ij folder correctly. However, the 24.0.12 build succeeds since the condition is not functioning as expected. Should we proceed with this change despite the failure in 24.0.9?

@anusreelakshmi934 I believe in our team discussion earlier today we agreed that the unzip step should be restored to allow the 24.0.9 build to run. We also agreed that a comment be added to document that the unzip step is required for compatibility with the 24.0.9 build.

I have added a comment above the unzip step.

Copy link
Contributor

@dessina-devasia dessina-devasia left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

Copy link
Contributor

@vaisakhkannan vaisakhkannan left a comment

Choose a reason for hiding this comment

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

Looks good

@anusreelakshmi934 anusreelakshmi934 merged commit b798331 into OpenLiberty:main Feb 12, 2025
3 of 4 checks passed
@anusreelakshmi934 anusreelakshmi934 deleted the localLsp4ijPathIssue branch February 13, 2025 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The LSP4IJ version is not being retrieved correctly when specifying the local path.
4 participants