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

Dynamic reports enhancement #111

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

pranavgaikwad
Copy link
Contributor

No description provided.

Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
@pranavgaikwad pranavgaikwad force-pushed the dynamicReports branch 3 times, most recently from 9481491 to 3533bc6 Compare March 21, 2023 15:43
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

I think this is an excellent start and does a good job of capturing the high-level needs.

I think I have some questions on specifics if we are planning on coming back and updating this and creating either enhancements or prs of the underlying pieces, that also could make a ton of sense. just let me know and resolve the conversations :)

Issues have following properties:

* Description: A text description in markdown format
* Rule ID: Rule for which the issue was created
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to change this to RuleSet.RuleID?

enhancements/dynamic-reports/README.md Show resolved Hide resolved
* Version: Version of the dependency.
* SHA: Checksum of the dependency.
* Indirect: Whether the dependency is indirect.
* Type: Type of the dependency.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is type for here? can you add an example


#### Historical data

Trends are generated from high level statistics of all analyses that happened so far. We want to store the computed statistics of every analysis and not the entire output. We want to limit the data points we store. But we are not defining a limit on the period of time for which the data stays. In this enhancement, we are saying all of the data from day 1 is available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we keeping every piece of data, OR are we saying it is only needed to keep the trend data say if it was calculated on each analysis run?

Personally, I would vote that we keep everything.

Copy link
Contributor Author

@pranavgaikwad pranavgaikwad Mar 22, 2023

Choose a reason for hiding this comment

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

@shawn-hurley I get what you're trying to say that we shouldn't be losing valuable data on purpose. But I also feel having too much data at user's disposal in the reports wouldn't be ideal from UX point of view. A good middle ground I feel would be - store most recent analysis data in Konveyor along with computed statistics, move raw data of previous runs to a secondary place but users don't see the raw old data in the report. This makes our life easy in getting the first iteration of reports out (where we dont care about old data), while not losing the old data in case we decide to do more intelligent things in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

From a high level that would be fine but does seem like we are agreeing that all data should be kept, just that it may not be available to the user through the UI. This makes sense to me can we more clearly codify that?


#### Application scope

##### Dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add the stretch goal of being able to see the whole graph? I assume this would change the data that is captured and reported in someway and I wonder if we need to make sure that we can upgrade to do this when we add it in the future?


#### Selection of applications

Since there could be thousands of applications in the inventory, displaying all issues / dependencies for all applications in the inventory might not be ideal for user experience. In this enhancement, we are proposing that the users select a subset of applications via a separate filter for which they want to see the list of issues or dependencies. They can also select all applications, but we want that to be the choice of the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a "may" or "must" select subset of applications?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawn-hurley honestly, I don't know. @rromannissen any thoughts?


#### Filters on multiple columns

One or more columns can be combined together to form complex searches. When filtering by multiple columns, logical AND between different types of columns is implied. Within the same type of columns, however, selecting multiple values implies a logical OR. For instance, when I select "mandatory" for the category column, and "java" and "golang" for target technology, it is implied that a list of issues with "mandatory" category AND with target technologies "java" OR "golang" will be returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if an example URL that says the UI and Hub could comment on for this might be good?

Are we going to implement something like graphql or will we use regular query strings?


* As a user, I want to be able to see all tags generated for the application by the analyzer.

* As a user, I want to be able to filter tags created by the analyzer by categories (tag types).
Copy link
Contributor

@jortel jortel Apr 7, 2023

Choose a reason for hiding this comment

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

As a user, I want to be able to see all tags generated for the application by the analyzer.
As a user, I want to be able to filter tags created by the analyzer by categories (tag types).

Is this different than existing tag display/filtering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jortel no. same as existing tag display

@shawn-hurley
Copy link
Contributor

I assume that either this is out of date or we need to merge. I think we need to get this up to date and merge if we are cutting the alpha.

@pranavgaikwad

@pranavgaikwad
Copy link
Contributor Author

@shawn-hurley not terribly out-of-date. Just need to move some things to future, as we decided to reduce the scope. Will update.

Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
@jwmatthews
Copy link
Member

@pranavgaikwad @shawn-hurley @jortel do you see further updates to this needed, or can we get an ACK and merge?

@pranavgaikwad
Copy link
Contributor Author

@jwmatthews I dont see any further updates.

@pranavgaikwad pranavgaikwad merged commit d4b6200 into konveyor:master Oct 24, 2023
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.

5 participants