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

feat: Expose validation webhook warnings in Argo CD UI #493

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Sergey-Kizimov
Copy link

@Sergey-Kizimov Sergey-Kizimov commented Dec 28, 2022

A warning handler has been added to this PR to display webhook validation warnings in the Argo CD user interface.
This PR is related to Issue posted before and PR in the ArgoCD repo.

Warnings in the ArgoCD UI will look like this:

Screen Shot 2022-12-28 at 2 43 40 PM

Screen Shot 2022-12-28 at 2 44 01 PM

@codecov
Copy link

codecov bot commented Dec 28, 2022

Codecov Report

Base: 55.75% // Head: 55.65% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (d4fd681) compared to base (917f5a0).
Patch coverage: 21.42% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
- Coverage   55.75%   55.65%   -0.11%     
==========================================
  Files          41       41              
  Lines        4525     4537      +12     
==========================================
+ Hits         2523     2525       +2     
- Misses       1808     1816       +8     
- Partials      194      196       +2     
Impacted Files Coverage Δ
pkg/sync/common/types.go 50.00% <0.00%> (-4.17%) ⬇️
pkg/sync/sync_context.go 72.75% <27.27%> (-0.60%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Signed-off-by: Sergey-Kizimov <sergey.kizimov@hiya.com>
Signed-off-by: Sergey-Kizimov <sergey.kizimov@hiya.com>
@sonarcloud
Copy link

sonarcloud bot commented Jan 13, 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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@ashutosh16 ashutosh16 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +540 to +541
case warning:
sc.setOperationPhase(common.OperationWarning, "synced with warning")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we do the other case successful stuff as well? i.e. delete hooks.

@NitriKx
Copy link

NitriKx commented Aug 8, 2023

Any news on this one?

@crenshaw-dev
Copy link
Collaborator

@NitriKx looks like the original author is unresponsive. I think my comment still needs to be addressed. Are you up to pick this PR up and run with it?

@Sergey-Kizimov
Copy link
Author

Sorry that I didn't answer for a long time, during my tests I found one bug, when synchronizing one object everything works correctly, but when synchronizing several objects (this should trigger several warnings), all warnings for objects are returned only for the last applied object, as I understood this is probably due to the fact that when synchronizing several objects, one session is used and several goroutines apply objects in parallel and for some reason there is no synchronization between the operation and the returned warning, I have no idea how to fix this. I need help with this

@Sergey-Kizimov
Copy link
Author

Sergey-Kizimov commented Aug 8, 2023

One hack that I see here is that we can publish warning messages in the operationState.message field without separating by resources, in this case it isn't necessary to care which resource caused the warning because all warnings will be merged. @crenshaw-dev What do you think?

@crenshaw-dev
Copy link
Collaborator

I haven't dived into the code, but it looks to me like there's a tasks list where the result of each task is stored. Maybe that's the appropriate place to handle/persist the webhook warning response?

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.

None yet

4 participants