Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

Preserve ArgoCD Notification state #193

Merged
merged 2 commits into from
Apr 13, 2021
Merged

Preserve ArgoCD Notification state #193

merged 2 commits into from
Apr 13, 2021

Conversation

dctrwatson
Copy link
Contributor

While #186 would probably be a long-term enhancement, this at least fixes #180 in the near term

@dctrwatson
Preserve ArgoCD Notification state
f932663 
Signed-off-by: John Watson <johnw@planetscale.com>
@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021 •
edited

CLA assistant check
All committers have signed the CLA.

@shkrid
Copy link

shkrid commented Apr 12, 2021 •
edited

I have built docker image from this PR
docker pull shkrid/applicationset:1.0.0-notification-fix
and tried, but issue was not resolved.

@dctrwatson

@dctrwatson
Copy link
Contributor Author

I have built docker image from this PR
docker pull shkrid/applicationset:1.0.0-notification-fix
and tried, but issue was not resolved.

Interesting... I'm also running an image from this PR and it is behaving as expected

The applicationset image I'm running is also based off this PR: argoproj-labs/argocd-notifications#241

But I'm not seeing any differences to the annotation key used by the notifications controller in recent history either, so not sure :(

jgwest
jgwest suggested changes Apr 12, 2021
View reviewed changes
Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

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

Thanks @dctrwatson , just one small tweak and good to go.

pkg/controllers/applicationset_controller.go Outdated
@@ -438,7 +445,16 @@ func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context,
action, err := utils.CreateOrUpdate(ctx, r.Client, found, func() error {
// Copy only the Application/ObjectMeta fields that are significant, from the generatedApp
found.Spec = generatedApp.Spec

// Preserve argo cd notifications state
Copy link
Member

Choose a reason for hiding this comment

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

Tiny tweak:
Can you change this line to

// Preserve argo cd notifications state (https://github.com/argoproj-labs/applicationset/issues/180)

(ie appending the issue # for some context)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍

OmerKahani
OmerKahani approved these changes Apr 13, 2021
View reviewed changes
Copy link
Contributor

@OmerKahani OmerKahani left a comment

Choose a reason for hiding this comment

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

Thanks!

jgwest
jgwest approved these changes Apr 13, 2021
View reviewed changes
Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @dctrwatson

@jgwest jgwest merged commit 67149a1 into argoproj:master Apr 13, 2021
@bitgandtter
Copy link

any ETA on this to be released?

@jgwest jgwest mentioned this pull request Jul 19, 2021
Closed
@pdrastil pdrastil mentioned this pull request Aug 10, 2021
Closed
@laloyalo laloyalo mentioned this pull request Sep 22, 2021
Closed
@lacarvalho91 lacarvalho91 mentioned this pull request Feb 7, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jgwest jgwest jgwest approved these changes

@OmerKahani OmerKahani OmerKahani approved these changes

@shkrid shkrid shkrid 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.

ApplicationSet conflicts with ArgoCD-Notification which cause notification spam
6 participants
@dctrwatson @CLAassistant @shkrid @bitgandtter @jgwest @OmerKahani