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: send informer add k8s event #2834

Merged
merged 14 commits into from
Jul 17, 2023
Merged

feat: send informer add k8s event #2834

merged 14 commits into from
Jul 17, 2023

Conversation

zachaller
Copy link
Collaborator

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@zachaller
add event for reconcilation
5e5cc7c 
Signed-off-by: zachaller <[email protected]>
@zachaller
add sprig
28d70e5 
Signed-off-by: zachaller <[email protected]>
@zachaller
fmt
9e4a059 
Signed-off-by: zachaller <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Jun 12, 2023 •
edited
Loading

Go Published Test Results

1 991 tests   1 991 ✔️  2m 36s ⏱️
   118 suites         0 💤
       1 files           0 ❌

Results for commit e2adb3f.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 12, 2023 •
edited
Loading

E2E Tests Published Test Results

    4 files      4 suites   3h 17m 10s ⏱️
  96 tests   86 ✔️   5 💤 5 ❌
394 runs  365 ✔️ 20 💤 9 ❌

For more details on these failures, see this check.

Results for commit 8108bb8.

♻️ This comment has been updated with latest results.

@zachaller
fix tests
ef611cf 
Signed-off-by: zachaller <[email protected]>
@zachaller
move event to add informer event
dc574b4 
Signed-off-by: zachaller <[email protected]>
@zachaller
remove bad code
f675d4a 
Signed-off-by: zachaller <[email protected]>
@codecov
Copy link

codecov bot commented Jun 12, 2023 •
edited
Loading

Codecov Report

Patch coverage: 72.72% and project coverage change: -0.04 ⚠️

Comparison is base (e94a380) 81.67% compared to head (8108bb8) 81.64%.

❗ Current head 8108bb8 differs from pull request most recent head e2adb3f. Consider uploading reports for the commit e2adb3f to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2834      +/-   ##
==========================================
- Coverage   81.67%   81.64%   -0.04%     
==========================================
  Files         133      133              
  Lines       20192    20202      +10     
==========================================
+ Hits        16492    16494       +2     
- Misses       2847     2853       +6     
- Partials      853      855       +2
Impacted Files Coverage Δ
utils/conditions/conditions.go 81.06% <ø> (ø)
rollout/controller.go 81.05% <72.72%> (-0.69%) ⬇️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zachaller zachaller changed the title feat: Add reconcile event feat: Add Informer Add Event Jun 12, 2023
@andyliuliming
Copy link
Contributor

not related to this PR, just want to ask is it possible to cherry-pick the ingress tls feature to 1.15 release? thanks @zachaller

@zachaller
handel bad ro objects
81fe368 
Signed-off-by: zachaller <[email protected]>
@zachaller
add new event
5e2f7a5 
Signed-off-by: zachaller <[email protected]>
@zachaller
typo
fa1c392 
Signed-off-by: zachaller <[email protected]>
@zachaller
switch to built in eventf
27f978f 
Signed-off-by: zachaller <[email protected]>
@zachaller zachaller changed the title feat: Add Informer Add Event feat: Send an informer add event Jun 14, 2023
@zachaller zachaller changed the title feat: Send an informer add event feat: Send an informer add kubernetes event Jun 14, 2023
@zachaller zachaller changed the title feat: Send an informer add kubernetes event feat: Send an informer add kubernetes event and enable sprig functions for notification engine Jun 14, 2023
@zachaller zachaller changed the title feat: Send an informer add kubernetes event and enable sprig functions for notification engine feat: Send informer add k8s event and enable sprig functions for notification engine Jun 14, 2023
@zachaller zachaller changed the title feat: Send informer add k8s event and enable sprig functions for notification engine feat: Send informer add k8s event Jun 21, 2023
@zachaller zachaller marked this pull request as ready for review June 23, 2023 16:59
@zachaller zachaller requested a review from leoluz July 17, 2023 14:55
leoluz
leoluz reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

Please check my comment

rollout/controller.go Outdated
EventReason: conditions.RolloutAddedToInformerReason,
}, "Rollout resource added to informer: %s/%s", ro.Namespace, ro.Name)
} else {
log.Warnf("Malformed rollout resource added to informer")
Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid that this message can be misleading if the cfg.Recorder is nil. Also, it seems that unstructuredutil.ObjectToRollout is already log warning if it fails to convert.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed it up a bit to depend on the log from ObjectToRollout for ro=nil and handled the Recorder being nil log

@sonarcloud
Copy link

sonarcloud bot commented Jul 17, 2023

Please retry analysis of this Pull-Request directly on SonarCloud.

@sonarcloud
Copy link

sonarcloud bot commented Jul 17, 2023

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
4.8% 4.8% Duplication

leoluz
leoluz approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

LGTM

@zachaller zachaller enabled auto-merge (squash) July 17, 2023 15:12
@zachaller zachaller changed the title feat: Send informer add k8s event feat: send informer add k8s event Jul 17, 2023
@zachaller zachaller merged commit 8db151f into argoproj:master Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leoluz leoluz leoluz approved these changes

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
@zachaller @andyliuliming @leoluz