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: oncePer condition only checked when when condition is true (#71) #208

Merged
merged 22 commits into from
Oct 19, 2023

Conversation

lol3909
Copy link
Contributor

@lol3909 lol3909 commented Aug 11, 2023

This change makes it that the oncePer condition only gets check if the when condition is true.
fixes #71

@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (b3e6b6a) 54.76% compared to head (8fd8df9) 54.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   54.76%   54.83%   +0.06%     
==========================================
  Files          35       35              
  Lines        3316     3321       +5     
==========================================
+ Hits         1816     1821       +5     
  Misses       1232     1232              
  Partials      268      268              
Files Coverage Δ
pkg/triggers/service.go 68.96% <72.72%> (+2.92%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lol3909 lol3909 marked this pull request as ready for review August 11, 2023 17:21
@mubarak-j
Copy link

@pasha-codefresh would you be able to review this PR?

@lol3909
Copy link
Contributor Author

lol3909 commented Oct 2, 2023

Hi, any news on this? :) @mubarak-j or @pasha-codefresh

@mubarak-j
Copy link

I pinged the maintainers on Slack https://cloud-native.slack.com/archives/C01TSERG0KZ/p1696269136126209
I'm just an end-user that hoping this long-standing annoying bug gets resolved, pretty sure will help save in log costs 💸 😄

} else {
log.Errorf("failed to execute when condition: %+v", err)
whenResult = false
Copy link
Member

Choose a reason for hiding this comment

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

whenResult can be false by default, so you will not need write this line

@@ -81,22 +81,29 @@ func (svc *service) Run(triggerName string, vars map[string]interface{}) ([]Cond
Templates: condition.Send,
Key: fmt.Sprintf("[%d].%s", i, hash(condition.When)),
}

var whenResult bool
log.Debug("This is the version with the whenResult variable")
Copy link
Member

Choose a reason for hiding this comment

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

i think this log is redundant

@pasha-codefresh
Copy link
Member

Hey @lol3909 , added few small comments

lol3909 and others added 20 commits October 19, 2023 08:19
Signed-off-by: Anthony Mikhail <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
This reverts commit 6313312.

Signed-off-by: Anthony Mikhail <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
…goproj#210)

Bumps [github.com/gregdel/pushover](https://github.com/gregdel/pushover) from 1.1.0 to 1.2.1.
- [Commits](gregdel/pushover@v1.1.0...v1.2.1)

---
updated-dependencies:
- dependency-name: github.com/gregdel/pushover
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Anthony Mikhail <[email protected]>
* Allow AutoMerge configuration

Signed-off-by: Mike Splain <[email protected]>

* Add docs and test

Signed-off-by: Mike Splain <[email protected]>

* Fix default and docs

Signed-off-by: Mike Splain <[email protected]>

---------

Signed-off-by: Mike Splain <[email protected]>
Co-authored-by: pasha-codefresh <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
* Add pull request comment to github service

Signed-off-by: bennesp <[email protected]>

* Add github pull request comment templater test

Signed-off-by: bennesp <[email protected]>

* Add github pull request comment docs

Signed-off-by: bennesp <[email protected]>

---------

Signed-off-by: bennesp <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 3 to 4.
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@v3...v4)

---
updated-dependencies:
- dependency-name: codecov/codecov-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Anthony Mikhail <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
Signed-off-by: Tom Paulus <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
…rgoproj#221)

* bump expr version

Signed-off-by: zachaller <[email protected]>

* upgrade go expr uses any

Signed-off-by: zachaller <[email protected]>

* use root go version

Signed-off-by: zachaller <[email protected]>

* add sum

Signed-off-by: zachaller <[email protected]>

* change for golang upgrade

Signed-off-by: zachaller <[email protected]>

---------

Signed-off-by: zachaller <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>

Signed-off-by: Anthony Mikhail <[email protected]>
* fix duplicate when using self-service

Signed-off-by: May Zhang <[email protected]>

* fix duplicate when using self-service

Signed-off-by: May Zhang <[email protected]>

* fix tests

Signed-off-by: May Zhang <[email protected]>

* -s

Signed-off-by: May Zhang <[email protected]>

* add test cases

Signed-off-by: May Zhang <[email protected]>

* remove not used method

Signed-off-by: May Zhang <[email protected]>

---------

Signed-off-by: May Zhang <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
* spelling: available

Signed-off-by: Josh Soref <[email protected]>

* spelling: configuration

Signed-off-by: Josh Soref <[email protected]>

* spelling: emoji

Signed-off-by: Josh Soref <[email protected]>

* spelling: environment

Signed-off-by: Josh Soref <[email protected]>

* spelling: github

Signed-off-by: Josh Soref <[email protected]>

* spelling: notification

Signed-off-by: Josh Soref <[email protected]>

* spelling: overridden

Signed-off-by: Josh Soref <[email protected]>

* spelling: successfully

Signed-off-by: Josh Soref <[email protected]>

* spelling: unmarshal

Signed-off-by: Josh Soref <[email protected]>

* spelling: variable or

Signed-off-by: Josh Soref <[email protected]>

* spelling: variables are

Signed-off-by: Josh Soref <[email protected]>

* spelling: webhook

Signed-off-by: Josh Soref <[email protected]>

---------

Signed-off-by: Josh Soref <[email protected]>
Co-authored-by: pasha-codefresh <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
* Clarify option types

Signed-off-by: Andreas Lindhé <[email protected]>

* Sort alphabetically

Signed-off-by: Andreas Lindhé <[email protected]>

* Add missing `channels` and `sigingSecret` options

Signed-off-by: Andreas Lindhé <[email protected]>

* Convert list to Markdown table

Signed-off-by: Andreas Lindhé <[email protected]>

* Clarify `token` description

Signed-off-by: Andreas Lindhé <[email protected]>

* Add example column

Signed-off-by: Andreas Lindhé <[email protected]>

* Populate rest of example fields

Signed-off-by: Andreas Lindhé <[email protected]>

* Fix typo

Co-authored-by: Blake Pettersson <[email protected]>
Signed-off-by: Andreas Lindhé <[email protected]>

---------

Signed-off-by: Andreas Lindhé <[email protected]>
Co-authored-by: Blake Pettersson <[email protected]>
Co-authored-by: pasha-codefresh <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
…proj#219)

Signed-off-by: Josh Soref <[email protected]>
Co-authored-by: pasha-codefresh <[email protected]>
Signed-off-by: Anthony Mikhail <[email protected]>
Copy link
Member

@pasha-codefresh pasha-codefresh left a comment

Choose a reason for hiding this comment

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

LGTM

@pasha-codefresh pasha-codefresh merged commit 5aca046 into argoproj:master Oct 19, 2023
5 checks passed
@lol3909 lol3909 deleted the fix-onceper-condition branch October 19, 2023 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oncePer condition logs expected error
10 participants