Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
kie-issues#1694: Apache KIE Extended Services extension issues #2802
kie-issues#1694: Apache KIE Extended Services extension issues #2802
Changes from 10 commits
a9d5f35
bfd6f96
1e4cbdb
a9970d3
497217f
3260a41
25b5662
ea6f438
1b86f15
6f2a072
3d83bf8
89a69b1
5d2b781
f9fdc5f
8eebccb
223e14d
bb98575
011beb5
34a0095
08bef0b
a7b083d
1bf8620
a556b1c
1e29bcb
51cb84d
3e9cd1b
fa14e13
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 use the
env
value instead of hardcoding. To do so, take a look into thebuild/defaultEnvJson.ts
file in some packages (e.g.online-editor
).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.
@ljmotta It's a good point. I would like to retrieve that default value directly from the configuration here https://github.com/apache/incubator-kie-tools/pull/2802/files#diff-153d43651b25ee3120020fae349c9e8ed99ccf90aa15995784702244decf9499R91 to avoid duplication and a single place for that property value, do you think is a valid alternative should I consider?
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.
yes, it is important to have an option to configure the url of extended services, users report issues about it #2759
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.
@ljmotta In the examples I checked, the env variables are accessed using the useEnv() hook, but in this case, we are not managing a React app. How can I access to env variable from "plain" typescript?
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.
@yesamer That's true. the
defaultEnvJson
is used with the webpack transform +fetch
, which isn't the case here. Now, you could use the webpack'sEnvironmentPlugin
to set env variables. Theonline-editor
uses this approach to set some values inside the codebase. Take a look into theonline-editor/webpack.config.ts
, and search forEnvironmentPlugin
. TheWEBPACK_REPLACE__*
values are the ones used in the codebase.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.
@ljmotta It should be done, there are two points to discuss:
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.
@yesamer
commonConfig
variable.extended-services-java
it will always usehttp
. If the user wants to deploy it, the user will need to make the necessary adjustments to make it available throughhttps
.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.
@ljmotta
web
andnode
https://github.com/apache/incubator-kie-tools/pull/2802/files#diff-1768335dedd5cfa5e1b3830bb3b6c504f1d316dea21de55738df74f7f19c27a5R52To create a single entry for the plugin, which
target
andentry
should I use?http
, we are safe here :)