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

Make results' limits configurable (fixes #347) #1331

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sskorol
Copy link

@sskorol sskorol commented Jun 23, 2021

Context

There were several places in code with hardcoded limits for items rendered on charts. It was really inconvenient for the end-user who had to rebuild the entire project to make limits configurable.

This fix moves the actual configuration into an Aggregator interface which is implemented by key plugins. Moreover, a user can now control the limits via ALLURE_RESULTS_LIMIT env variable that could be set before the report's generation. Note that a newly added method was intentionally made static as some of the APIs that adjust the limits are also static.

Fixes #347

Checklist

@indradeepchowdhury
Copy link
Contributor

Hello, Just checking to see if there is a possibility of this PR to get merged soon and what it would take to get this merged :)

Thank you.

Copy link
Contributor

@indradeepchowdhury indradeepchowdhury left a comment

Choose a reason for hiding this comment

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

LGTM!

@sskorol
Copy link
Author

sskorol commented Nov 11, 2021

@indradeepchowdhury as far as I remember, @baev had some concerns in terms of implementation. Need to double-check with him.

@indradeepchowdhury
Copy link
Contributor

@indradeepchowdhury as far as I remember, @baev had some concerns in terms of implementation. Need to double-check with him.

Thanks for the update @sskorol. @baev would it be possible to expedite this feature. This would be a super cool feature to have.

@pbandura
Copy link

Hi @sskorol. Thanks for jour work on providing such feature.
I've got one question - do you still work on this or has it been parked?

@sskorol
Copy link
Author

sskorol commented Dec 20, 2021

Hi @pbandura, I don't. When I initially pushed it, it was a fully working feature. I talked to maintainers directly. And they promised to check. Seems like they didn't have time. Now I see there's a conflict. I can resolve it. And then you will be able to build a new version with this feature from sources if it's important for you.

@indradeepchowdhury
Copy link
Contributor

Hi @sskorol I think this feature would be pretty cool and I am looking forward to using it. This feature will allow us to align the history with our Sprint duration or even let us increase the history limit to a month for proper tracking 🙂

There were several places in code with hardcoded limits for items rendered on charts. It was really inconvenient for the end-user who had to rebuild the entire project to make limits configurable.

This fix moves the actual configuration into an Aggregator interface which is implemented by key plugins. Moreover, a user can now control the limits via ALLURE_RESULTS_LIMIT env variable that could be set before report's generation. Note that a newly added method was intentionally made static as some of the APIs that adjust the limits are also static.
@sskorol sskorol force-pushed the 347-make-trends-limit-configurable branch from 790340f to 50c63b0 Compare December 21, 2021 09:13
@sskorol
Copy link
Author

sskorol commented Dec 21, 2021

@indradeepchowdhury @pbandura force-pushed changes with conflicts' resolution. Now you can build the report on your own with this feature.

@sangheee
Copy link

sangheee commented Oct 5, 2022

@sskorol @indradeepchowdhury @baev Why didn't you guys apply this?

@baev
Copy link
Member

baev commented Oct 5, 2022

I don't like the current implementation because:

  1. Plugin API changes -- new method introduced in Aggregator interface may create some issues for other users
  2. The env variable processing is unsafe -- You can set negative or non-number env variable and crash your report generation
  3. There are no tests provided

Besides that, our team is heavily focused on Allure 3 development right now, so this change isn't really in priority right now

Copy link

@A1exKH A1exKH left a comment

Choose a reason for hiding this comment

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

Code LGTM.

Please resolve conflicts for this PR.

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

Successfully merging this pull request may close these issues.

Make number of items in history and history trend configurable
6 participants