-
Notifications
You must be signed in to change notification settings - Fork 11
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
FUSETOOLS2-2314: Provide extra parameter when running and debugging with JBang #615
Conversation
4ae02e8
to
90e5818
Compare
7d35eb4
to
e12b1c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- we should be able to clear the setting, the parameter is marked as invalid
- currently it is supporting a single option at a time, if we provide several either they are ignored or are providing an error, for instance by ^providing
--console --health
as value, there is this error:
Executing task: jbang '-Dcamel.jbang.version=4.5.0' camel@apache/camel run Demo.camel.yaml --dev --logging-level=info '--console --health'
2024-04-04 13:44:30.752 INFO 113860 --- [ main] .download.MavenDependencyDownloader : Resolved: org.apache.camel:camel-jbang-plugin-k:4.5.0 (took: 3s589ms)
Unknown option: '--console --health'
Possible solutions: --console, --code
* The terminal process "/usr/bin/bash '-c', 'jbang '-Dcamel.jbang.version=4.5.0' camel@apache/camel run Demo.camel.yaml --dev --logging-level=info '--console --health''" terminated with exit code: 2.
it very probably comes from the backtick quotes enclosing both of the values
and thought about it when closing the video call, it is worthy to provide an entry in the changelog and in the readme/doc |
e12b1c3
to
780509d
Compare
afterEach(async function () { | ||
await cleanEnvironment(); | ||
resetUserSettings(EXTRA_LAUNCH_PARAMETER_ID); | ||
}); | ||
|
||
it('Should use default extra launch parameter', async function () { | ||
await executeCommand(CAMEL_RUN_ACTION_QUICKPICKS_LABEL); | ||
|
||
await waitUntilTerminalHasText(driver, [`${defaultExtraLaunchParameter}`], 6000, 120000); | ||
}); | ||
|
||
it(`Should use user defined extra launch parameter'${customExtraLaunchParameter}'`, async function () { | ||
await setJBangVersion(customExtraLaunchParameter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the other tests are failing because the Camel version setting is modified to --fresh and not set back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The tests are failing only for vscode version 1.82.3. I tested locally with this version and they run without errors:
$ CODE_VERSION=1.82.3 npm run ui-test
> [email protected] preui-test
> npm run compile
> [email protected] compile
> tsc -p ./ && npm run lint
> [email protected] lint
> eslint src --ext ts
> [email protected] ui-test
> node --unhandled-rejections=warn-with-error-code out/ui-test/uitest_runner.js
WARNING: You are using the outdated VS Code version '1.82.3'. The latest stable version is '1.88.0'.
Downloading VS Code: 1.82.3 / stable
VS Code exists in local cache, skipping download
ChromeDriver 114.0.5735.90 exists in local cache, skipping download
Executing prepublish script 'npm run vscode:prepublish'...
> [email protected] vscode:prepublish
> npm run compile
> [email protected] compile
> tsc -p ./ && npm run lint
> [email protected] lint
> eslint src --ext ts
This extension consists of 849 files, out of which 591 are JavaScript files. For performance reasons, you should bundle your extension: https://aka.ms/vscode-bundle-extension . You should also exclude unnecessary files by adding them to your .vscodeignore: https://aka.ms/vscode-vscodeignore
DONE Packaged: /home/mdinizde/Development/Projects/camel-dap-client-vscode/vscode-debug-adapter-apache-camel-0.13.0.vsix (849 files, 5.92MB)
Installing extensions...
(node:1359519) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
(Use `code --trace-deprecation ...` to show where the warning was created)
Extension 'vscode-debug-adapter-apache-camel-0.13.0.vsix' was successfully installed.
Detected user defined code settings
Writing code settings to /tmp/test-resources/settings/User/settings.json
Launching browser...
Browser ready in 4185 ms
Launching tests...
Camel User Settings
Update Camel Version
✔ Should use '3.20.1' user defined Camel version (24207ms)
Update JBang Version
✔ Should use default JBang version (9669ms)
✔ Should use user defined JBang version '3.20.5' (15272ms)
Update Maven Repository
✔ Should use '3.20.1.redhat-00026' user defined Camel Version and Red Hat Maven Repository (15173ms)
✔ Should not use '#repos' placeholder for global Camel JBang repository config (20615ms)
Update Extra Launch Parameter
✔ Should use default extra launch parameter (9559ms)
✔ Should use user defined extra launch parameter'--fresh' (15244ms)
Shutting down the browser
Uninstalling redhat.vscode-debug-adapter-apache-camel...
Extension 'redhat.vscode-debug-adapter-apache-camel' was successfully uninstalled!
7 passing (3m)
- By looking at the screenshots of the failing tests for the windows build it seems that the test isnot able to retrieve the JBang version property in the settings page.
- For the ubuntu build I could not find anything relevant in the screenshots.
b2d8e93
to
5fc5969
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 tests are still failing (with different errors):
- VS Code 1.82.3 on Linux
- VS Code 1.82.3 on Windows (based on screenshot, potential reasons: notification popup behind the properties which were moved to due new ones in the UI, if that is the reason, it can be workarounded by providing the correct settings avoiding to open the notification)
Either need to fix that or we try to upgrade the version with which we are testing to a more recent one. As a rule of thumb, we try to have at least 2 versions old and only as best effort for more.
src/task/CamelJBangTaskProvider.ts
Outdated
`${this.getRedHatMavenRepository()}`, | ||
{ | ||
"value": `${this.getExtraLaunchParameter()}`, | ||
"quoting": 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Enum constant instead of a "magical number", it will allow to understand slightly better to what it corresponds.
src/task/CamelJBangTaskProvider.ts
Outdated
`${this.getRedHatMavenRepository()}`, | ||
{ | ||
"value": `${this.getExtraLaunchParameter()}`, | ||
"quoting": 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Enum constant instead of a "magical number", it will allow to understand slightly better to what it corresponds.
src/task/CamelJBangTaskProvider.ts
Outdated
`${this.getRedHatMavenRepository()}`, | ||
{ | ||
"value": `${this.getExtraLaunchParameter()}`, | ||
"quoting": 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What lead to this choice of Weak Shell Quoting instead of keeping default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option do not work with multiple parameters:
- quoting=1:
--param1\ --param2
(does not work) - quoting=2:
'--param1 --param2'
(does not work) - quoting=3:
"'--param1' '--param2'"
(works if passing them with single quotes)
…ith JBang Signed-off-by: Marcelo Henrique Diniz de Araujo <[email protected]>
5fc5969
to
cbed5b4
Compare
|
Tried with newer VS Code version and the tests are passing: #619 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
providing several parameters is still not working. There is no error thrown but the parameters are ignored for instance when using '--console' '--health'
Possibilities:
- mention in the description that it works for a single parameter only and implement for several in another iteration
- find a way to make it working, maybe the easiest is to provide a setting which is a list of string so that we do not have to take care ourself of parsing the parameters and proviing them one by one to the ShellExecution
1.82.3 is quite out of supported versions by ExTester already so if it is passing with newer I would go with that |
covered by #623 based on the work done in this PR |
Signed-off-by: Marcelo Henrique Diniz de Araujo [email protected]