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.

Sign up for GitHub

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

Jump to bottom

Split too big comment per cluster #22

Merged
merged 13 commits into from
Sep 12, 2024
Merged

Split too big comment per cluster #22

merged 13 commits into from
Sep 12, 2024

Conversation

Oded-B
Copy link
Collaborator

@Oded-B Oded-B commented Aug 30, 2024 •
edited
Loading

Description

This pull request introduces several changes to the internal/pkg/githubapi package, focusing on improving the handling of ArgoCD diff comments and enhancing the template execution process. The key changes include splitting multi cluster diff comment per cluster.

  • created a new function to generate the comment/s based rendered template size.
  • Added a loop to run over all created comments and "send" them to GH API
  • Added an optional Header to the template to allow adding a number for each comment.
  • Added a Test + Testdata

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change
  • Refactor
  • Documentation
  • Other (please describe)

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests
  • Functionality is documented
  • All code style checks pass
  • New code contribution is covered by automated tests
  • All new and existing tests pass

@Oded-B
Split too big comment per cluster
932d46e 
If a "regular" aggrigated diff can't fit in a GH comment create one
comment per cluster.
There's still a fallback to concise diff (just lists changed objects)
for extreme cases
@Oded-B
Don't continue to sad path unless needed
0b6347b
@Oded-B
Check return value
68d9fd0
@Oded-B
Move logging out of executeTemplate to higher up the stack
8ea0c3c 
Create a new "testable"  generateArgoCdDiffComments function to generate
all the comments content

Comment all the comments
@Oded-B
Add test for 3 "levels" of comments
39fed16
@Oded-B
Merge remote-tracking branch 'origin/main' into split_comment_by_cluster
d8149eb
@Oded-B
Replace depricated package
ef24e05
@Oded-B Oded-B marked this pull request as ready for review September 5, 2024 14:16
hnnsgstfssn
hnnsgstfssn reviewed Sep 10, 2024
View reviewed changes
internal/pkg/githubapi/github.go Outdated Show resolved Hide resolved
internal/pkg/githubapi/github.go Outdated Show resolved Hide resolved
internal/pkg/githubapi/github.go Outdated Show resolved Hide resolved
internal/pkg/githubapi/github.go Outdated Show resolved Hide resolved
internal/pkg/githubapi/github.go Show resolved Hide resolved
internal/pkg/githubapi/github.go Outdated Show resolved Hide resolved
internal/pkg/githubapi/github.go Outdated Show resolved Hide resolved
internal/pkg/githubapi/github.go Outdated Show resolved Hide resolved
internal/pkg/githubapi/github.go Outdated Show resolved Hide resolved
internal/pkg/githubapi/github.go Outdated Show resolved Hide resolved
Oded-B and others added 3 commits September 10, 2024 11:22
@Oded-B @hnnsgstfssn
Apply suggestions from code review
fe297c4 
Co-authored-by: Hannes Gustafsson <hnnsgstfssn@gmail.com>
@Oded-B @hnnsgstfssn
Apply suggestions from code review
5003d5b 
Co-authored-by: Hannes Gustafsson <hnnsgstfssn@gmail.com>
@Oded-B
Fix typo in var name
3327c7b
hnnsgstfssn
hnnsgstfssn previously approved these changes Sep 10, 2024
View reviewed changes
templates/argoCD-diff-pr-comment.gotmpl Show resolved Hide resolved
hnnsgstfssn
hnnsgstfssn previously approved these changes Sep 10, 2024
View reviewed changes
internal/pkg/githubapi/github_test.go Show resolved Hide resolved
templates/argoCD-diff-pr-comment.gotmpl Show resolved Hide resolved
internal/pkg/githubapi/github.go Outdated Show resolved Hide resolved
templates/argoCD-diff-pr-comment.gotmpl Show resolved Hide resolved
yzdann
yzdann previously approved these changes Sep 11, 2024
View reviewed changes
internal/pkg/githubapi/github.go Show resolved Hide resolved
@Oded-B Oded-B dismissed stale reviews from yzdann and hnnsgstfssn via c90127d September 12, 2024 07:25
@Oded-B
Apply suggestions from code review
f79e70a 
Co-authored-by: Hannes Gustafsson <hnnsgstfssn@gmail.com>

Remove some uneeded comments
Simplify tests function

Address lint issue
@Oded-B Oded-B merged commit d60f34e into main Sep 12, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hnnsgstfssn hnnsgstfssn Awaiting requested review from hnnsgstfssn

@yzdann yzdann Awaiting requested review from yzdann

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Oded-B @hnnsgstfssn @yzdann