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

feat: basic implementation for inclusions #445

Merged
merged 21 commits into from
Jul 4, 2024
Merged

feat: basic implementation for inclusions #445

merged 21 commits into from
Jul 4, 2024

Conversation

furkansb
Copy link
Contributor

@furkansb furkansb commented Jun 7, 2024

@davidebianchi how do you feel about adding support for inclusion rules? I think it might be useful for some teams/companies where they have to use some independent services in the same namespaces. At least for my team, this is definitely the case.

If you find it ok, I'll add tests and complete the PR. I wanted to get your opinion first.

davidebianchi
davidebianchi reviewed Jun 8, 2024
View reviewed changes
Copy link
Member

@davidebianchi davidebianchi left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I definitely think it would be a nice feature to add an include list!

internal/controller/sleepinfo/jsonpatch/genericresources.go Outdated Show resolved Hide resolved
@davidebianchi davidebianchi added the enhancement New feature or request label Jun 8, 2024
@davidebianchi davidebianchi mentioned this pull request Jun 17, 2024
Open
@furkansb
Copy link
Contributor Author

furkansb commented Jun 17, 2024 •
edited
Loading

@davidebianchi I pushed some changes to allow both inclusions and exclusions. I also added some tests but if you need more test coverage just let me know.

I specifically included a test case where the inclusion is set with a label but the resource is excluded based on its name. It passes the tests. I'll be waiting your review!

@coveralls
Copy link

coveralls commented Jun 17, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9554721163

Details

  • 38 of 41 (92.68%) changed or added relevant lines in 2 files are covered.
  • 35 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.7%) to 78.366%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/sleepinfo_types.go 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
api/v1alpha1/zz_generated.deepcopy.go 35 64.75%
Totals Coverage Status
Change from base Build 9488619699: -0.7%
Covered Lines: 1199
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 17, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9554727447

Details

  • 38 of 41 (92.68%) changed or added relevant lines in 2 files are covered.
  • 35 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.7%) to 78.366%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/sleepinfo_types.go 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
api/v1alpha1/zz_generated.deepcopy.go 35 64.75%
Totals Coverage Status
Change from base Build 9488619699: -0.7%
Covered Lines: 1199
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 17, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9554747820

Details

  • 38 of 41 (92.68%) changed or added relevant lines in 2 files are covered.
  • 71 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-3.1%) to 76.013%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/sleepinfo_types.go 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
api/v1alpha1/zz_generated.deepcopy.go 35 64.75%
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -3.1%
Covered Lines: 1163
Relevant Lines: 1530
💛 - Coveralls

davidebianchi
davidebianchi requested changes Jun 23, 2024
View reviewed changes
Copy link
Member

@davidebianchi davidebianchi left a comment

Choose a reason for hiding this comment

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

Thank you for the enhancement, really useful! Some comments. Can you add also an e2e test on the include feature?

charts/kube-green/Chart.yaml Outdated Show resolved Hide resolved
internal/controller/sleepinfo/jsonpatch/genericresources.go Outdated Show resolved Hide resolved
internal/controller/sleepinfo/jsonpatch/genericresources.go Show resolved Hide resolved
internal/controller/sleepinfo/jsonpatch/genericresources.go Show resolved Hide resolved
api/v1alpha1/sleepinfo_types.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9648970109

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • 36 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-2.9%) to 76.209%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Files with Coverage Reduction New Missed Lines %
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -2.9%
Covered Lines: 1166
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 24, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9653084867

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 78.562%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Totals Coverage Status
Change from base Build 9488619699: -0.5%
Covered Lines: 1202
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 25, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9658054514

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • 36 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-2.9%) to 76.209%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Files with Coverage Reduction New Missed Lines %
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -2.9%
Covered Lines: 1166
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 25, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9658225371

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • 36 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-2.9%) to 76.209%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Files with Coverage Reduction New Missed Lines %
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -2.9%
Covered Lines: 1166
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 26, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9677983397

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 78.562%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Totals Coverage Status
Change from base Build 9488619699: -0.5%
Covered Lines: 1202
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 26, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9678156387

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • 36 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-2.9%) to 76.209%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Files with Coverage Reduction New Missed Lines %
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -2.9%
Covered Lines: 1166
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 26, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9679613368

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 78.562%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Totals Coverage Status
Change from base Build 9488619699: -0.5%
Covered Lines: 1202
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 26, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9679799079

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • 36 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-2.9%) to 76.209%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Files with Coverage Reduction New Missed Lines %
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -2.9%
Covered Lines: 1166
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 26, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9680074655

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 78.562%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Totals Coverage Status
Change from base Build 9488619699: -0.5%
Covered Lines: 1202
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 26, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9682276028

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 78.562%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Totals Coverage Status
Change from base Build 9488619699: -0.5%
Covered Lines: 1202
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 26, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9682616964

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • 36 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-2.9%) to 76.209%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Files with Coverage Reduction New Missed Lines %
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -2.9%
Covered Lines: 1166
Relevant Lines: 1530
💛 - Coveralls

@furkansb
Copy link
Contributor Author

Thank you for the enhancement, really useful! Some comments. Can you add also an e2e test on the include feature?

@davidebianchi I did but I guess either sth is wrong with my implementation because e2e tests do not work for inclusion setup. However, all the unit tests I have added passes. So maybe sth else needs to be configured to support this. Do you see anything that might be breaking e2e tests? Asking cause the logic is quite simple.

davidebianchi
davidebianchi reviewed Jun 29, 2024
View reviewed changes
Copy link
Member

@davidebianchi davidebianchi left a comment

Choose a reason for hiding this comment

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

In this way, test should run. In this assert, instead of wait until resources are stopped, it is ok to check only that the manifest is changed accordingly.

tests/e2e/inclusions/01-assert-sleep.yaml Outdated Show resolved Hide resolved
tests/e2e/inclusions/01-assert-sleep.yaml Outdated Show resolved Hide resolved
@furkansb @davidebianchi
Apply suggestions from code review
31adafb 
Co-authored-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
@coveralls
Copy link

coveralls commented Jun 30, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9730538422

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • 36 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-2.9%) to 76.209%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Files with Coverage Reduction New Missed Lines %
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -2.9%
Covered Lines: 1166
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 30, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9730783899

Details

  • 42 of 62 (67.74%) changed or added relevant lines in 3 files are covered.
  • 36 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-2.9%) to 76.209%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 1 21 4.76%
Files with Coverage Reduction New Missed Lines %
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -2.9%
Covered Lines: 1166
Relevant Lines: 1530
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 30, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9732699504

Details

  • 63 of 82 (76.83%) changed or added relevant lines in 4 files are covered.
  • 50 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-3.5%) to 75.551%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 18 37 48.65%
Files with Coverage Reduction New Missed Lines %
api/v1alpha1/zz_generated.deepcopy.go 14 59.74%
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -3.5%
Covered Lines: 1165
Relevant Lines: 1542
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 30, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9732731287

Details

  • 63 of 82 (76.83%) changed or added relevant lines in 4 files are covered.
  • 14 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-1.2%) to 77.886%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 18 37 48.65%
Files with Coverage Reduction New Missed Lines %
api/v1alpha1/zz_generated.deepcopy.go 14 59.74%
Totals Coverage Status
Change from base Build 9488619699: -1.2%
Covered Lines: 1201
Relevant Lines: 1542
💛 - Coveralls

@furkansb
Copy link
Contributor Author

@davidebianchi all the requested changes are implemented. I believe it should be fine to merge now. Looking forward to use this in production!

davidebianchi
davidebianchi requested changes Jul 2, 2024
View reviewed changes
Copy link
Member

@davidebianchi davidebianchi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Only a small change! It should also be removed the ExcludeRef struct. Once removed, run make generate to update the autogenerated files

api/v1alpha1/sleepinfo_types.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 2, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9757326940

Details

  • 53 of 55 (96.36%) changed or added relevant lines in 4 files are covered.
  • 39 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-2.2%) to 76.918%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 8 10 80.0%
Files with Coverage Reduction New Missed Lines %
api/v1alpha1/zz_generated.deepcopy.go 3 72.58%
internal/controller/sleepinfo/internal/mocks/cronjob.go 36 57.47%
Totals Coverage Status
Change from base Build 9488619699: -2.2%
Covered Lines: 1163
Relevant Lines: 1512
💛 - Coveralls

davidebianchi
davidebianchi approved these changes Jul 4, 2024
View reviewed changes
Copy link
Member

@davidebianchi davidebianchi left a comment

Choose a reason for hiding this comment

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

Great, thank you for your work! LGTM

@davidebianchi davidebianchi merged commit abc57ad into kube-green:main Jul 4, 2024
17 checks passed
@furkansb
Copy link
Contributor Author

furkansb commented Jul 4, 2024

Great, thank you for your work! LGTM

Awesome! If you can make an rc release when appropriate, that'd be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidebianchi davidebianchi davidebianchi approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@furkansb @coveralls @davidebianchi