-
Notifications
You must be signed in to change notification settings - Fork 36
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
Containerless Kantra Distribution for Java Analysis #196
Conversation
Signed-off-by: Emily McMullan <[email protected]>
67dd018
to
309a023
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.
I like the idea overall. I am requesting that we include all pre-requisites for the user to be able to run this on their machines. Also, can we talk about Builtin provider too?
|
||
Users will be expected to have or install several tools onto their machines, such as OpenJDK and Maven. We will produce and ship our [rulesets](https://github.com/konveyor/rulesets/), JDTLS, and the [Java Bundle](https://github.com/konveyor/java-analyzer-bundle). | ||
|
||
The user will be instructed to set these packaged binaries and rules in a known location, such as `$HOME/.kantra/`. Alternatively, an install script can set these. |
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.
By setting, do you mean keeping those binaries there? Can we use standard system paths as well as environment variables for this?
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.
For JDT.LS and java analyzer bundle, for unistall purposes it would be nice I think if this was all in .kantra/<folders>
for ease of use, Maven, OpenJDK should be system paths and we use the correct env vars ($M2_ and $JAVA_) IMO
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.
Are we going to allow these to be in the system path?
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.
Install script to add .kantra/<folders>/...
to path? Or users add it manually? And set those as env vars in kantra? And +1 for using expected env vars for Maven and OpenJDK. @djzager Are you instead thinking of using the binaries from the abs path?
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 I meant was that, if a user installs the binary at /usr/local/bin
then it should be accessible via Kantra right?
I suppose that what I am suggesting is that we may install stuff at ${XDG_CONFIG_HOME}/.kantra/...
but we should encourage the users to set their PATH
to include that directory so kantra
the CLI is free to just execute them (and not go looking for them or expecting environment variables).
|
||
### Technical Implmentation | ||
|
||
A new analyze subcommand can be introduced for kantra to recognize when to start the containerized version of analysis versus the non-containerized option. This could look like `kantra analyze-java-only <options>`. Another option for starting the analyze subcommand without running containers would be to detect existing packaged requirements at the expected location discussed in the [requirements](#requirements). However, in this case, we would want to provide an option to disable this behavior. |
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 like the idea of having an explicit / separate subcommand for this. Keeps things clean for us and users. Think we should discuss more about semantics, analyze-java-only sounds restrictive. Maybe fine right now, but unsure if similar requirement will arise for other providers.
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.
It is "restricte" in that you can't use any other providers (besides builtin which you called out we need to add :) good catch!)
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 wish I could come up with a decent analyze-no-container
name that could be more generic.
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.
Yea, I am still not sure of this subcommand name, so I'm open to any other suggestions.
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 best I could come up with kantra analyze-bin --provider=java
, gives us ability to expand to other providers.
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 am a +1 on @djzager's flag to add more, or the one in the enhancement
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 this genuinely improves on what I've heard previously discussed. I do somewhat wonder what prevents us from creating a new command altogether (separate go module) and whether that may give us some more flexibility even if in the same repository.
|
||
Users will be expected to have or install several tools onto their machines, such as OpenJDK and Maven. We will produce and ship our [rulesets](https://github.com/konveyor/rulesets/), JDTLS, and the [Java Bundle](https://github.com/konveyor/java-analyzer-bundle). | ||
|
||
The user will be instructed to set these packaged binaries and rules in a known location, such as `$HOME/.kantra/`. Alternatively, an install script can set these. |
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.
Are we going to allow these to be in the system path?
|
||
### Technical Implmentation | ||
|
||
A new analyze subcommand can be introduced for kantra to recognize when to start the containerized version of analysis versus the non-containerized option. This could look like `kantra analyze-java-only <options>`. Another option for starting the analyze subcommand without running containers would be to detect existing packaged requirements at the expected location discussed in the [requirements](#requirements). However, in this case, we would want to provide an option to disable this behavior. |
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 wish I could come up with a decent analyze-no-container
name that could be more generic.
"I think this genuinely improves on what I've heard previously discussed. I do somewhat wonder what prevents us from creating a new command altogether (separate go module) and whether that may give us some more flexibility even if in the same repository." |
Signed-off-by: Emily McMullan <[email protected]>
05f06bd
to
92a549a
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.
Understanding why a new InternalProviderClient and not just importaing from external is the only open question in my book
Otherwise this LGTM
|
||
### Technical Implmentation | ||
|
||
A new analyze subcommand can be introduced for kantra to recognize when to start the containerized version of analysis versus the non-containerized option. This could look like `kantra analyze-java-only <options>`. Another option for starting the analyze subcommand without running containers would be to detect existing packaged requirements at the expected location discussed in the [requirements](#requirements). However, in this case, we would want to provide an option to disable this behavior. |
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 am a +1 on @djzager's flag to add more, or the one in the enhancement
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.
This looks good to me. The only two (non-blocking) comments I have are:
- To consider documenting that we will start shipping binaries as the release artifacts for kantra (and any other necessary component) as part of this work.
- Consider
kantra analyze-bin --provider=java
as a way to satisfy the requirements with room to grow in the future.
Signed-off-by: Emily McMullan <[email protected]>
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.
ACK
Signed-off-by: Emily McMullan <[email protected]>
Closes #188