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

No Notification for "Completed" and without controller Pods #485

Merged
merged 10 commits into from
Oct 27, 2023
Merged

No Notification for "Completed" and without controller Pods #485

merged 10 commits into from
Oct 27, 2023

Conversation

fbm3307
Copy link
Contributor

@fbm3307 fbm3307 commented Oct 19, 2023

If the Pod is in complete state and doesn't have a controller, no notification should be sent to the user.

This relates to #478

@fbm3307
No Notification for "Completed" and without controller Pods
058d122 
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
rajivnathan
rajivnathan reviewed Oct 19, 2023
View reviewed changes
Copy link
Contributor

@rajivnathan rajivnathan left a comment

Choose a reason for hiding this comment

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

Looks good, just a few questions. Also, we should add some unit tests for this.

controllers/idler/idler_controller.go Outdated Show resolved Hide resolved
controllers/idler/idler_controller.go Outdated Show resolved Hide resolved
@fbm3307
Merge branch 'master' into notifyemailchange
7d14f30
@fbm3307
addressing the review comments
80d43e8 
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
@fbm3307
Adding a unit test case
8c75087 
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
@fbm3307
Copy link
Contributor Author

fbm3307 commented Oct 23, 2023 •
edited
Loading

Also, we should add some unit tests for this.

Added the Unit test case for this.

@fbm3307 fbm3307 requested a review from rajivnathan October 23, 2023 10:22
@fbm3307
Fixing the duplication
826d302 
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
@codecov
Copy link

codecov bot commented Oct 23, 2023 •
edited
Loading

Codecov Report

Merging #485 (6e7e7a4) into master (68b24f2) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 6e7e7a4 differs from pull request most recent head e427f36. Consider uploading reports for the commit e427f36 to get more accurate results

@@            Coverage Diff             @@
##           master     #485      +/-   ##
==========================================
+ Coverage   82.40%   82.44%   +0.03%     
==========================================
  Files          30       30              
  Lines        3172     3178       +6     
==========================================
+ Hits         2614     2620       +6     
  Misses        426      426              
  Partials      132      132
Files Coverage Δ
controllers/idler/idler_controller.go 93.27% <100.00%> (+0.09%) ⬆️

@fbm3307
Adding testcases for controlled and completed pod status notify
56bf1e7 
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
@fbm3307
Copy link
Contributor Author

fbm3307 commented Oct 25, 2023

I will update the test cases to reduce duplication

alexeykazakov
alexeykazakov reviewed Oct 26, 2023
View reviewed changes
Copy link
Contributor

@alexeykazakov alexeykazakov left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few minor comments for test improvement.

controllers/idler/idler_controller_test.go Outdated Show resolved Hide resolved
controllers/idler/idler_controller_test.go Show resolved Hide resolved
controllers/idler/idler_controller_test.go Show resolved Hide resolved
@fbm3307
addresing review comments for test cases
6e7e7a4 
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
@fbm3307 fbm3307 requested a review from alexeykazakov October 27, 2023 04:47
@fbm3307
Adding multiple conditions type for test
e427f36 
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

alexeykazakov
alexeykazakov approved these changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@alexeykazakov alexeykazakov left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you for addressing the comments.

controllers/idler/idler_controller_test.go Show resolved Hide resolved
@openshift-ci
Copy link

openshift-ci bot commented Oct 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, fbm3307

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@alexeykazakov alexeykazakov merged commit 6669ed5 into codeready-toolchain:master Oct 27, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexeykazakov alexeykazakov alexeykazakov approved these changes

@MatousJobanek MatousJobanek Awaiting requested review from MatousJobanek MatousJobanek is a code owner

@xcoulon xcoulon Awaiting requested review from xcoulon xcoulon is a code owner

@sbryzak sbryzak Awaiting requested review from sbryzak

@mfrancisc mfrancisc Awaiting requested review from mfrancisc mfrancisc is a code owner

@ranakan19 ranakan19 Awaiting requested review from ranakan19 ranakan19 is a code owner

@rajivnathan rajivnathan Awaiting requested review from rajivnathan rajivnathan is a code owner

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

Successfully merging this pull request may close these issues.
4 participants
@fbm3307 @alexeykazakov @rajivnathan @ranakan19