search

konveyor / tackle2-ui

Tackle (2nd generation) UI component.
Apache License 2.0
8 stars 43 forks source link

:bug: Change default analyze mode to source + dependencies #1819

Closed sjd78 closed 7 months ago

sjd78 commented 7 months ago

Resolves: #1364

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

Resolve eslint warnings in touched files.

sjd78 commented 7 months ago

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

sjd78 commented 7 months ago

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

On check re-run, everything passed.

:man_shrugging:

ibolton336 commented 7 months ago

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?

sjd78 commented 7 months ago

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

Reordered in the latest update

sjd78 commented 7 months ago

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:

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

ibolton336 commented 7 months ago

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.

sjd78 commented 7 months ago

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 https://github.com/konveyor/tackle2-ui/issues/1837 to capture the discussion as a future enhancement.