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

Automatically suggest updates based on JDT-LS Java language support. #3402

Merged
merged 22 commits into from
Jan 12, 2024

Conversation

mavaddat
Copy link
Contributor

@mavaddat mavaddat commented Nov 27, 2023

I created this action and script by closely scrutinizing the logic of the pull requests for this and the eclipse-jdtls/eclipse.jdt.ls repos:

This led me to generalize the workflow using the very convenient API endpoints for the eclipse-jdtls CI pipeline.

I tested the scripts on my local machine, but of course, the resources and tokens on the runner are different. Needs testing.

Checks for JDK updates, issues PR
Create check_and_update_jdk.py
To manually trigger a GitHub Action, you need to add a `workflow_dispatch` event to your workflow file. This event allows you to trigger the workflow manually using the GitHub Actions web interface or the GitHub API¹.
After adding the `workflow_dispatch` event, you can manually trigger your GitHub Action by going to the Actions tab of your repository and clicking on the "Run workflow" button¹. Please note that to trigger the `workflow_dispatch` event, your workflow must be in the default branch¹. If your workflow is on a different branch, you might need to merge it into the default branch first. Also, write access to the repository is required to perform these steps¹..

Sources:
(1) Manually running a workflow - GitHub Docs. https://docs.github.com/en/actions/using-workflows/manually-running-a-workflow.
(2) Triggering a workflow - GitHub Docs. https://docs.github.com/en/actions/using-workflows/triggering-a-workflow.
(3) Manual Trigger In GitHub Actions - Knoldus Blogs. https://blog.knoldus.com/manual-trigger-in-github-actions/.
(4) Configuring Manual Triggers In GitHub Actions With ... - Build5Nines. https://build5nines.com/configuring-manual-triggers-in-github-actions-with-workflow_dispatch/.
(5) Manually Trigger a GitHub Action with workflow_dispatch. https://dev.to/this-is-learning/manually-trigger-a-github-action-with-workflowdispatch-3mga.
No need to emit an error
Move disk write conditional on updating the JDK version.
@mavaddat
Copy link
Contributor Author

I have tested this action on my fork. I believe peter-evans/create-pull-request@v5 action requires that the Allow GitHub Actions to create and approve pull requests setting be enabled on the repository.

Actions settings · vscode-java Screenshot

This setting can be found in a repository's settings under Actions > General > Workflow permissions. For repositories belonging to an organization, this setting can be managed by admins in organization settings under Actions > General > Workflow permissions. This is necessary because the action needs the appropriate permissions to create a pull request on our behalf.

Tests are `PASSED`, not `SUCCESS`
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

So you'd like to automate the creation of PRs like 31f7f3f . That could be nice. Ultimately it would depend on the support being merged into JDT-LS as you've discovered. And those depend on support in JDT Core (eg. 21, 22).

CC'ing @fbricon @snjeza for awareness.

.github/workflows/bump-jdk.yml Outdated Show resolved Hide resolved
.github/workflows/bump-jdk.yml Outdated Show resolved Hide resolved
.github/scripts/check_and_update_jdk.py Show resolved Hide resolved
Use gh instead of 3rd party PR action
Update regex logic to replace `java.configuration.runtimes` example as well:

```json
  {
    "name": "JavaSE-21",
    "path": "/path/to/jdk-21",
    "default": true
  },
```
Correct minor errors:

- In the `Branch and push changes` step, had `with` keyword instead of the `run` keyword leftover from previous implementation.
- In the same step, changed the `::set-env` command to set the `latest_jdk` environment variable to use the `env` keyword instead, as described in this [guide][1]. This command is deprecated and will be removed soon.
 
[1] https://docs.github.com/en/actions
Utilize shebang line at the top.
@mavaddat
Copy link
Contributor Author

mavaddat commented Nov 29, 2023

Ultimately it would depend on the support being merged into JDT-LS as you've discovered. And those depend on support in JDT Core (eg. 21, 22).

Do we need JDT Core to add this as a feature? It seemed to me that we can track the tests that are already published in the CI:

Num Package Class Method Latest test result
1 correction ModifierCorrectionsQuickFixTest testAddSealedMissingClassModifierProposal Link
2 correction ModifierCorrectionsQuickFixTest testAddSealedAsDirectSuperClass Link
3 correction ModifierCorrectionsQuickFixTest testAddPermitsToDirectSuperClass Link
4 correction UnresolvedTypesQuickFixTest testTypeInSealedTypeDeclaration Link
5 managers EclipseProjectImporterTest testPreviewFeaturesDisabledByDefault Link
6 managers InvisibleProjectImporterTest testPreviewFeaturesEnabledByDefault Link
7 managers MavenProjectImporterTest testJava21Project Link

The last test method and endpoint is variable with the latest_jdk, which is how I parameterized it in the Python script.

@rgrunber
Copy link
Member

No, I just wanted to explain that the changes that go into JDT-LS that are tracked in the script by certain test cases depend on JDT Core supporting that particular Java version.

Forgot how gfm links work 😅
Changing comment to reflect PR technique
Prevent the workflow from running if an equivalent pull request (PR) has already been issued by checking for a PR with the same title
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Apologies for the delay. Tried this out and it looks good. Just a small change I noticed is needed to adjust to some changes we've made since.

If you don't have time to adjust it, let me know and I could take over as it seems to be in working order.

.github/scripts/check_and_update_jdk.py Outdated Show resolved Hide resolved
> Since [b1d0831](redhat-developer@b1d0831) , we took advantage of [settings groups](https://code.visualstudio.com/docs/getstarted/settings#_settings-groups), which made the configuration object contain a list of the groups, each containing a properties objects.
@rgrunber rgrunber self-assigned this Jan 10, 2024
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Based on my last attempt, this looks ready to merge. We're just preparing a release, and after that, we can add this in.

Clean up previous `latest_jdk.txt` records prior to checking for its existence to not get false positives.
@rgrunber rgrunber merged commit d5536ac into redhat-developer:master Jan 12, 2024
2 checks passed
@rgrunber rgrunber added this to the End January 2024 milestone Jan 12, 2024
@rgrunber rgrunber changed the title Automatically track JDK of eclipse.jdt.ls Automatically suggest updates based on JDT-LS Java language support. Jan 12, 2024
@mavaddat mavaddat deleted the patch-1 branch January 13, 2024 05:32
@rgrunber
Copy link
Member

@mavaddat , Just a heads-up the script didn't work for eclipse-jdtls/eclipse.jdt.ls#3111. Apparently the regex won't catch just 22 at https://javadl-esd-secure.oracle.com/update/baseline.version .

Also, not sure why I didn't catch this before, but the checks in the script, from the table above, that confirm JDT-LS has new version support and that reference "Sealed" should not be used. Sealed classes were finalized in Java 17 but for some reason it probably looked like their version should continue to be bumped. Probably just the 3 importer related tests are enough.

@mavaddat mavaddat restored the patch-1 branch March 26, 2024 20:15
@mavaddat
Copy link
Contributor Author

@mavaddat , Just a heads-up the script didn't work for eclipse-jdtls/eclipse.jdt.ls#3111. Apparently the regex won't catch just 22 at https://javadl-esd-secure.oracle.com/update/baseline.version .

Right. I see why. I did not know that major releases could have no minor semver part. I have corrected this in the 49e3b48 commit.

Also, not sure why I didn't catch this before, but the checks in the script, from the table above, that confirm JDT-LS has new version support and that reference "Sealed" should not be used. Sealed classes were finalized in Java 17 but for some reason it probably looked like their version should continue to be bumped. Probably just the 3 importer related tests are enough.

Oh, that's too bad. I kind of liked the loop I created to check for those tests. 😅 I have removed them nevertheless on 49e3b48.

@rgrunber
Copy link
Member

rgrunber commented Mar 27, 2024

Yeah that format looked odd to me as well, but feel free to propose a PR for it.

It also published this a few hours ago.. I suspect because of :

 Latest JDK version: 21
Current supported JDK version: 22
New JDK version detected: 21

at #3547 (hide whitespace makes the change easier to understand). I guess we should have our package.json be properly formatted to have the changes easier to see in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants