feat: add application link on PR diff comment#336
feat: add application link on PR diff comment#336dag-andersen merged 12 commits intodag-andersen:mainfrom
Conversation
|
@dag-andersen Displaying the link is optional, but in the commit below I changed the display style of the diff comment.
|
There was a problem hiding this comment.
Overall, I really like this! 🎉 @ponkio-o
I totally understand why you chose to make the diff prettier with the separate header, file ref, and "Click-me" button - it does look nice and clean!
That said, I'm a bit concerned about the extra space it takes up. At my workplace, we often have changes to 10+ applications per PR, which means the diff can get pretty long even when everything's collapsed. So I'd love to keep things as compact as possible! (especially if the user did not specify a argocd-ui-url)
I would prefer to keep the old format, but with an added link like this:
example-1 (link) (examples/internal-chart/app.yaml)
<details>
<summary>example-3 (<a href="argocd.example.com/applications/internal-chart-example">link</a>) (examples/internal-chart/app.yaml)</summary>
<br>
example-2 (link) (examples/internal-chart/app.yaml)
<details>
<summary>example-2 (<a href="argocd.example.com/applications/internal-chart-example">link</a>) (examples/internal-chart/app.yaml)</summary>
<br>
example-3 (link) (examples/internal-chart/app.yaml)
<details>
<summary>example-3 (<a href="argocd.example.com/applications/internal-chart-example">link</a>) (examples/internal-chart/app.yaml)</summary>
<br>
This keeps all the important info but saves a ton of screen real-estate. What do you think? 😄
Co-authored-by: Dag Andersen <dagbjerreandersen@gmail.com>
|
@dag-andersen Thank you for your review!
I see. That approach does seem to preserve the original style. I've made the fix in the commit below. |
pkg/diff/html.go
Outdated
| <summary> | ||
| %header% | ||
| </summary> | ||
| <summary>%summary%</summary> |
There was a problem hiding this comment.
| <summary>%summary%</summary> | |
| <summary> | |
| %summary% | |
| </summary> |
You will have to revert this formatting, because otherwise the integration tests fails :)
I just ran make check-release from your branch and noticed it 🚀
There was a problem hiding this comment.
I just ran make check-release from your branch and noticed it 🚀
I'm sorry, I didn't know that command existed.
When I ran it, a timeout occurred, but I don't know why 🤔
🤷 No secrets folder found at ./secrets
🦑 Installing Argo CD Helm Chart version: 'latest'
🦑 Installed Chart version: '9.3.7' and App version: 'v3.2.6'
🦑 Argo CD Helm chart installed successfully
🦑 Logging in to Argo CD through CLI...
🦑 Argo CD installed successfully in 41s
🤖 Converting ApplicationSets to Applications for both branches
❌ Failed to generate applications from ApplicationSet error="failed to run argocd appset generate: argocd command timed out after 60 seconds: signal: killed" (AppSet: "git-generator-example-appset [b|examples/git-generator/app/app-set.yaml]") (branch: integration-test/branch-5/base)
❌ Failed to generate apps (branch: integration-test/branch-5/base)
❌ Failed to generate base apps (branch: integration-test/branch-5/base)
❌ Failed to generate apps from ApplicationSets
🧟 Cluster will be kept alive after the tool finishes
❌ failed to run argocd appset generate: argocd command timed out after 60 seconds: signal: killed
🕵️ Run with '--debug' for more details
integration_test.go:452: Failed to run tool: exit status 1
❌ Stopping test run due to failure
=== NAME TestIntegration
integration_test.go:323: Cleaning up: deleting kind cluster...
Deleting cluster "argocd-diff-preview" ...
Deleted nodes: ["argocd-diff-preview-control-plane"]
--- FAIL: TestIntegration (372.51s)
--- FAIL: TestIntegration/branch-5/target-8 (371.74s)
FAIL
FAIL github.com/dag-andersen/argocd-diff-preview/integration-test 372.675s
FAIL
make[1]: *** [run-integration-tests-go] Error 1
make: *** [check-release] Error 2
There was a problem hiding this comment.
No that should not be related 😕
can you run make run-with-go target_branch=helm-example-3, or does that also fail?
The integration tests are basically just 28x2 runs of argocd-diff-preview run as a binary and as a docker image.
I am running make check-release from your branch now.
There was a problem hiding this comment.
and it passed all the integration-test so i'll merge later today 🚀 🎉
There was a problem hiding this comment.
can you run make run-with-go target_branch=helm-example-3, or does that also fail?
It's passed!
and it passed all the integration-test so i'll merge later today 🚀 🎉
🎉
Co-authored-by: Dag Andersen <dagbjerreandersen@gmail.com>



Summary
This PR adds the ability to generate clickable links to ArgoCD applications in the diff output by introducing a new
--argocd-ui-urlflag. This flag is optional, and doesn't show links when this flag is not provided.issue: #331
Usage
# Generate diff with clickable links argocd-diff-preview --argocd-ui-url https://argocd.example.com ...Example
When

--argocd-ui-urlis set, a link to the Argo CD Application page is added, as shown below.About Application URL
URL style is
<namespace>/<name>on "Application in any namespace". But<name>style is also supported for backwards compatibility.https://argo-cd.readthedocs.io/en/stable/operator-manual/app-any-namespace/#application-names
If argocd-diff-preview officially supports Applications in any namespace, please let me know as this requires consideration.