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.

Sign up for GitHub

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

Parameterization for Running the LTI Build Fixes #783 #833

Closed
wants to merge 1 commit into from
Closed

Parameterization for Running the LTI Build Fixes #783 #833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion .github/workflows/build.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ name: Build

on:
workflow_dispatch:
inputs:
useLocalPlugin:
description: 'Use lsp4ij locally'
required: true
default: 'false'
push:
branches: '**'
pull_request:
Expand All @@ -24,6 +29,8 @@ jobs:
- runtime: windows
os: windows-latest
reportName: windows-test-report
env:
USE_LOCAL_PLUGIN: ${{ github.event.inputs.useLocalPlugin || 'false' }}
steps:
- name: Configure pagefile
if: contains(matrix.os, 'windows')
Expand All @@ -39,9 +46,28 @@ jobs:
- name: 'Install required integration test software'
working-directory: ./liberty-tools-intellij
run: bash ./src/test/resources/ci/scripts/setup.sh

# Checkout and build lsp4ij only if USE_LOCAL_PLUGIN is true
- name: 'Checkout lsp4ij'
if: env.USE_LOCAL_PLUGIN == 'true'
uses: actions/checkout@v3
with:
repository: redhat-developer/lsp4ij
path: lsp4ij
ref: main
- name: 'Build Lsp4ij'
if: env.USE_LOCAL_PLUGIN == 'true'
working-directory: ./lsp4ij
run: bash ./gradlew buildPlugin
- name: 'Unzip lsp4ij file'
if: env.USE_LOCAL_PLUGIN == 'true'
working-directory: ./lsp4ij/build/distributions
run: |
unzip -o '*.zip' -d .
Comment on lines +51 to +66
Copy link
Contributor

@mrglavas mrglavas Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I know I might be getting confused by the number of prototypes we've had for these workflows but I thought our continuous integration builds would support building from PRs as well as the current main branch. I'm not seeing that logic here. Do we have this somewhere else (on another branch or PR)? Can it all be integrated into the same workflow and controlled by a build parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know I might be getting confused by the number of prototypes we've had for these workflows but I thought our continuous integration builds would support building from PRs as well as the current main branch. I'm not seeing that logic here. Do we have this somewhere else (on another branch or PR)? Can it all be integrated into the same workflow and controlled by a build parameter?

This ticket was primarily for determining the parameterization required to run the LTI builds. For easy prototyping, I tested with checking out the main branch only. There is a separate ticket for running the LTI tests against each open lsp4ij PR.


- name: 'Build Liberty-Tools-Intellij'
working-directory: ./liberty-tools-intellij
run: bash ./gradlew buildPlugin
run: bash ./gradlew buildPlugin -PuseLocal=${{ env.USE_LOCAL_PLUGIN }}
- name: 'Archive artifacts'
if: ${{ runner.os == 'Linux' && !failure() }}
uses: actions/upload-artifact@v3
Expand Down
5 changes: 4 additions & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,10 @@ intellij {
// For a full list of IntelliJ IDEA releases please see https://www.jetbrains.com/intellij-repository/releases
version = '2024.1.3'

plugins = ['java', 'maven', 'gradle-java', 'properties', 'terminal', 'org.jetbrains.idea.maven', 'com.intellij.gradle', 'com.redhat.devtools.lsp4ij:' + lsp4ijVersion + '@nightly']
def lsp4ij = project.hasProperty('useLocal') && project.property('useLocal') == 'true' ?
file("../lsp4ij/build/distributions/LSP4IJ/") :
'com.redhat.devtools.lsp4ij:' + lsp4ijVersion + '@nightly'
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you didn't change this in this PR, but noticed this is hard-coded here to pull the LSP4iJ version from the Nightly channel. When we release we definitely want to be depending on a Stable release of LSP4iJ for our build and testing. I wonder if we could separate that in build.gradle like the lsp4ijVersion variable that's being read here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know you didn't change this in this PR, but noticed this is hard-coded here to pull the LSP4iJ version from the Nightly channel. When we release we definitely want to be depending on a Stable release of LSP4iJ for our build and testing. I wonder if we could separate that in build.gradle like the lsp4ijVersion variable that's being read here.

Since we have not yet decided which version of Lsp4ij to use for our release, we are currently specifying the nightly version. Once we decide to proceed with the stable release, we can remove the @nightly tag and specify the stable version. For now, we are using @nightly to test our functionalities. When we move forward with our release, we will switch to the stable version.

Copy link
Contributor

@TrevCraw TrevCraw Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@anusreelakshmi934 I think @mrglavas 's suggestion is to parameterize the build channel the same way the lsp4ijVersion is parameterized - i.e. the values would be @nightly or @stable. That way we can just update the variable at the top of build.gradle when switching from nightly to stable or vice versa.

Copy link
Contributor

@mrglavas mrglavas Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@TrevCraw That is exactly what I was suggesting. @anusreelakshmi934 I hope that's clearer now.

Copy link
Contributor Author

@anusreelakshmi934 anusreelakshmi934 Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. Got it. I have updated the lsp4ijVersion variable at the top of build.gradle by appending @nightly along with the version number. If we are to use the stable version we can remove that @nightly and just specify the version alone.

plugins = ['java', 'maven', 'gradle-java', 'properties', 'terminal', 'org.jetbrains.idea.maven', 'com.intellij.gradle', lsp4ij]
updateSinceUntilBuild = false
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/resources/ci/scripts/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ main() {

# Start the IDE.
echo -e "\n$(${currentTime[@]}): INFO: Starting the IntelliJ IDE..."
./gradlew runIdeForUiTests --info > remoteServer.log 2>&1 &
./gradlew runIdeForUiTests -PuseLocal=$USE_LOCAL_PLUGIN --info > remoteServer.log 2>&1 &

# Wait for the IDE to come up.
echo -e "\n$(${currentTime[@]}): INFO: Waiting for the Intellij IDE to start..."
Expand All @@ -172,7 +172,7 @@ main() {

# Run the tests
echo -e "\n$(${currentTime[@]}): INFO: Running tests..."
./gradlew test
./gradlew test -PuseLocal=$USE_LOCAL_PLUGIN

# If there were any errors, gather some debug data before exiting.
rc=$?
Expand Down
Loading