-
Notifications
You must be signed in to change notification settings - Fork 43
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
🐛 Change default analyze mode to source + dependencies #1819
Conversation
Initial fail in the e2e test: |
On check re-run, everything passed. 🤷♂️ |
Seems to work OK. Only nit is seems odd to have the third item in the dropdown selected by default. Maybe we could rearrange the options? |
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.
Move Source + deps option to the first item in the dropdown list for analysis mode
Reordered in the latest update |
Resolves: konveyor#1364 Change the default analysis mode from "binary" to "source-code-deps". Resolve eslint warnings in touched files. Adjust unit tests. Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[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.
OTOH, it is exactly the same as what happens now. I'm conflicted since the issue just asks for the default to change. I like your idea of changing the source for analysis that is selected by default to "none" if there is an initial validation error. I'd even go one step further and auto select based on how the app itself is configured:
So either I can update the issue with the new ask, or create a new issue to detail the further enhancement. WDYT? |
Yeah
Up to you - You're right no regression introduced here / it is currently not working in this way as it stands today. A new issue does sound like a preferable way to go. With that said - LGTM. |
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.
LGTM
Opened #1837 to capture the discussion as a future enhancement. |
Resolves: #1364
Change the default analysis mode from "binary" to "source-code-deps".
Resolve eslint warnings in touched files.