Skip to content
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

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

sjd78
Copy link
Member

@sjd78 sjd78 commented Apr 4, 2024

Resolves: #1364

Change the default analysis mode from "binary" to "source-code-deps".

Resolve eslint warnings in touched files.

@sjd78 sjd78 requested review from ibolton336 and rszwajko April 4, 2024 22:41
@sjd78 sjd78 force-pushed the default-analysis branch from 4f147ea to 8ffac14 Compare April 4, 2024 23:06
@sjd78
Copy link
Member Author

sjd78 commented Apr 5, 2024

Initial fail in the e2e test: migration/applicationinventory/analysis/source_analysis_without_creds.test.ts

@sjd78
Copy link
Member Author

sjd78 commented Apr 5, 2024

Initial fail in the e2e test: migration/applicationinventory/analysis/source_analysis_without_creds.test.ts

On check re-run, everything passed.

🤷‍♂️

@sjd78 sjd78 force-pushed the default-analysis branch from 8ffac14 to 636fb1c Compare April 5, 2024 19:26
@sjd78 sjd78 added this to the v0.4.0 milestone Apr 5, 2024
@ibolton336
Copy link
Member

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?

Copy link
Member

@ibolton336 ibolton336 left a 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

@sjd78 sjd78 force-pushed the default-analysis branch from ef9c08b to 035399f Compare April 9, 2024 21:09
@sjd78 sjd78 requested a review from ibolton336 April 9, 2024 21:09
@sjd78
Copy link
Member Author

sjd78 commented Apr 9, 2024

Move Source + deps option to the first item in the dropdown list for analysis mode

Reordered in the latest update

sjd78 added 2 commits April 9, 2024 17:10
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]>
@sjd78 sjd78 force-pushed the default-analysis branch from 035399f to 2014ae7 Compare April 9, 2024 21:10
Copy link
Member

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-04-10 at 9 34 17 AM

There seems to be a validation hole when source + deps analysis is selected for an app with no source info filled out on the form. Maybe we should just add a placeholder for the mode and have the default value be null? That way the validation message will show when the user makes a selection.

@sjd78
Copy link
Member Author

sjd78 commented Apr 10, 2024

There seems to be a validation hole when source + deps analysis is selected for an app with no source info filled out on the form. Maybe we should just add a placeholder for the mode and have the default value be null? That way the validation message will show when the user makes a selection.

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:

  • Binary definitions only = binary
  • Source definitions only = source + dependencies
  • Source and Binary definitions = source + dependencies
  • No definitions = no default

So either I can update the issue with the new ask, or create a new issue to detail the further enhancement. WDYT?

@ibolton336
Copy link
Member

Yeah

There seems to be a validation hole when source + deps analysis is selected for an app with no source info filled out on the form. Maybe we should just add a placeholder for the mode and have the default value be null? That way the validation message will show when the user makes a selection.

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:

  • Binary definitions only = binary
  • Source definitions only = source + dependencies
  • Source and Binary definitions = source + dependencies
  • No definitions = no default

So either I can update the issue with the new ask, or create a new issue to detail the further enhancement. WDYT?

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.

Copy link
Member

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sjd78
Copy link
Member Author

sjd78 commented Apr 11, 2024

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.

Opened #1837 to capture the discussion as a future enhancement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Default analysis mode: source+deps.
3 participants