-
Notifications
You must be signed in to change notification settings - Fork 112
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
Avoid unnecessary use of reflection #214
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
public Void call() { | ||
EnvVars.masterEnvVars.putAll(enVars); |
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.
Not sure how this would work with multiple calls to EnvInjectMasterEnvVarsSetter#call
. This very well may be incorrect. 😕
@basil Yes we could test it on our DevOps dev env |
Hello, sorry was on vacation w\o access to internet. We try this version on our Test instance, for 2 weeks, and got now issues so far. |
Hello!
|
I saw the recent changes to make this plugin work on Java 17 but I was a bit dissatisfied with having to add
--add-opens
arguments. I think the plugin can be simplified to avoid the use of reflection entirely, which simplifies maintenance and obviates the need for--add-opens
directives.I tested this by running the automated tests and doing some basic tests locally on both the controller and on an agent, but I have not done very exhaustive manual testing and this code is very fragile, so I would appreciate any additional testing that others can offer. @bulanovk are you interested in testing this out in production for a week or so and reporting back if there are any issues?