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

fix: avoid performance degradation with Github and --hide-prev-plan-comments enabled #5241

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

oleg-glushak
Copy link

what

Switch from GitHub REST API to GitHub GraphQL for listing comments to expose the isMinimized attribute to avoid minimizing already minimized comments on each Atlantis command execution.

why

This helps to avoid performance degradation by minimizing only non-minimized Atlantis comments, as opposed to processing all comments sequentially on each Atlantis command execution.

tests

  • I have tested my changes by running unit tests.
  • I have tested my changes by running this version of Atlantis and checking if the --hide-prev-plan-comments performance still works in general and the performance degradation disappears.

references

Closes #5232

@oleg-glushak oleg-glushak requested review from a team as code owners January 17, 2025 06:19
@oleg-glushak oleg-glushak requested review from chenrui333, lukemassa and X-Guardian and removed request for a team January 17, 2025 06:19
@github-actions github-actions bot added go Pull requests that update Go code provider/github labels Jan 17, 2025
@oleg-glushak oleg-glushak changed the title Improve Github --hide-prev-plan-comments performance fix: Improve Github --hide-prev-plan-comments performance Jan 17, 2025
@oleg-glushak oleg-glushak changed the title fix: Improve Github --hide-prev-plan-comments performance fix: avoid performance degradation with Github and --hide-prev-plan-comments enabled Jan 17, 2025
@oleg-glushak oleg-glushak force-pushed the speed-up-github-comments-hide3 branch from 30b722f to 4f5b17f Compare January 17, 2025 06:21
@dosubot dosubot bot added the feature New functionality/enhancement label Jan 17, 2025
@X-Guardian
Copy link
Contributor

Thanks for this @oleg-glushak. Can you check the linting error: https://github.com/runatlantis/atlantis/pull/5241/files#annotation_30986616047

@X-Guardian X-Guardian added the waiting-on-response Waiting for a response from the user label Jan 17, 2025
Signed-off-by: oleg-glushak <[email protected]>
Signed-off-by: oleg-glushak <[email protected]>
Signed-off-by: oleg-glushak <[email protected]>
Signed-off-by: oleg-glushak <[email protected]>
Signed-off-by: oleg-glushak <[email protected]>
Signed-off-by: oleg-glushak <[email protected]>
@oleg-glushak oleg-glushak force-pushed the speed-up-github-comments-hide3 branch from 485dde2 to 6b0868f Compare January 17, 2025 14:02
@oleg-glushak
Copy link
Author

Thanks @X-Guardian

The pull requests is ready for review again.

@X-Guardian
Copy link
Contributor

I'm not keen on just ignoring the linter with a generic message. Can you investigate further? Also, can you try and avoid force pushing changes to the branch if you can, as this loses the links to previous check runs.

@oleg-glushak
Copy link
Author

I'm not keen on just ignoring the linter with a generic message. Can you investigate further? Also, can you try and avoid force pushing changes to the branch if you can, as this loses the links to previous check runs.

Thanks for your feedback @X-Guardian . The same approach was used in a pull request merged last month: here. As far as I understand we either have to ignore this warning or switch from int to int32 in many places including the interface here which would make the scope of these changes much bigger.

Please let me know, if you have a strong opinion which way I should follow or if I understand something wrong.

@oleg-glushak
Copy link
Author

@X-Guardian @jamengual I'd be happy to invest more time to push the pull request forward. Just let me know, if you have a strong opinion, if we need to do further work here.

@jamengual jamengual added waiting-on-review Waiting for a review from a maintainer and removed waiting-on-response Waiting for a response from the user labels Jan 24, 2025
@X-Guardian
Copy link
Contributor

I'm not keen on just ignoring the linter with a generic message. Can you investigate further? Also, can you try and avoid force pushing changes to the branch if you can, as this loses the links to previous check runs.

Thanks for your feedback @X-Guardian . The same approach was used in a pull request merged last month: here. As far as I understand we either have to ignore this warning or switch from int to int32 in many places including the interface here which would make the scope of these changes much bigger.

Please let me know, if you have a strong opinion which way I should follow or if I understand something wrong.

Can you use the same lint annotation as the previous PR then and add a TODO comment to the client.go file.

@X-Guardian X-Guardian added waiting-on-response Waiting for a response from the user and removed waiting-on-review Waiting for a review from a maintainer labels Jan 25, 2025
@oleg-glushak
Copy link
Author

@X-Guardian Done. The pull request is ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement go Pull requests that update Go code provider/github waiting-on-response Waiting for a response from the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github: Gradual performance degradation with --hide-prev-plan-comments enabled
3 participants