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

Make https optional #40

Merged
merged 2 commits into from
Dec 1, 2023
Merged

Make https optional #40

merged 2 commits into from
Dec 1, 2023

Conversation

aaron-weisberg-qz
Copy link
Contributor

@aaron-weisberg-qz aaron-weisberg-qz commented Dec 1, 2023

Motivation

  • Moving to decentralized ArgoCD per cluster paradigm, we can now use and internal address to get ArgoCD diffs
  • Action needs to take into account multiple diff comments for different argocd instances per commit
    • The current behavior to overwrite of existing comments causes race conditions and ultimately only a single diff gets posted
    • We also need to each diff to identity which cluster it's from

Changes

  • Adds a new plaintext input which bypasses the need for a verifiable cert (which is also not setup in the new cluster)
  • Adds a new environment input for customizing the diff comment title
  • Posts new diff comments on each commit
    • Open to suggestions on this, but thought the need to look at old comments to get the latest diff is confusing
    • Would rather delete outdate ones and alway post a new comment

Future work

  • We need to migrate away from using local diffs --local to server-side diffs since this option is being deprecated
  • There are also additional performance optimizations that we should probably make before doing that though (since it causes lots of manifests generations on each PR commit on the remote argocd instances (e.x. repo-server).

Tagging

  • I think we should tag master before merging just so legacy runs can easily revert
  • We should also tag post merge with an upgrade so we can start locking to a tag

@aaron-weisberg-qz aaron-weisberg-qz merged commit 0eabe5d into master Dec 1, 2023
0 of 2 checks passed
@aaron-weisberg-qz aaron-weisberg-qz deleted the make-https-optional branch December 1, 2023 17:51
@rgoldfinger-quizlet
Copy link
Contributor

Thanks for these improvements and I feel bad you're having to go through my old cold 😅

Would rather delete outdate ones and alway post a new comment

When implementing this I got a lot of complaints about always posting a new comment, since it spams people's email with very large messages. That's why it was moved to replacing the old comment instead.

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.

4 participants