From bc83ec370625f57d8e0dbce3a57ed3a2b7c72a21 Mon Sep 17 00:00:00 2001
From: Seth Vargo <seth@sethvargo.com>
Date: Fri, 27 May 2022 16:23:39 -0400
Subject: [PATCH] Ensure pinning is idempotent (#22)

Previously, running "pin" on an already-pinned file would lose the original ratchet references.
---
 command/pin.go         |  2 +-
 parser/actions.go      | 17 ++++++++---------
 parser/actions_test.go |  2 ++
 parser/parser.go       |  8 ++++++++
 parser/parser_test.go  | 27 ++++++++++++++++++++++++++-
 5 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/command/pin.go b/command/pin.go
index 709379e11d..c16a9cde8a 100644
--- a/command/pin.go
+++ b/command/pin.go
@@ -21,7 +21,7 @@ hashed version for the given input file:
     actions/checkout@v3 -> actions/checkout@2541b1294d2704b0964813337f...
 
 The original unpinned version is preserved in a comment, next to the pinned
-version.
+version. If a version is already pinned, it does nothing.
 
 To update versions that are already pinned, use the "update" command instead.
 
diff --git a/parser/actions.go b/parser/actions.go
index 487d674faa..ca80aeb651 100644
--- a/parser/actions.go
+++ b/parser/actions.go
@@ -91,17 +91,16 @@ func (a *Actions) Parse(m *yaml.Node) (*RefsList, error) {
 							for k, property := range step.Content {
 								if property.Value == "uses" {
 									uses := step.Content[k+1]
-									// Only include references to remove workflows. This could be a
-									// local workflow, which should not be pinned.
+									// Only include references to remove workflows. This could be
+									// a local workflow, which should not be pinned.
 									switch {
+									case strings.HasPrefix(uses.Value, "docker://"):
+										ref := resolver.NormalizeContainerRef(uses.Value)
+										refs.Add(ref, uses)
 									case strings.Contains(uses.Value, "@"):
 										ref := resolver.NormalizeActionsRef(uses.Value)
 										refs.Add(ref, uses)
-									case strings.Contains(uses.Value, "docker://"):
-										ref := resolver.NormalizeContainerRef(uses.Value)
-										refs.Add(ref, uses)
 									}
-									break
 								}
 							}
 						}
@@ -114,12 +113,12 @@ func (a *Actions) Parse(m *yaml.Node) (*RefsList, error) {
 						// Only include references to remove workflows. This could be a
 						// local workflow, which should not be pinned.
 						switch {
+						case strings.HasPrefix(uses.Value, "docker://"):
+							ref := resolver.NormalizeContainerRef(uses.Value)
+							refs.Add(ref, uses)
 						case strings.Contains(uses.Value, "@"):
 							ref := resolver.NormalizeActionsRef(uses.Value)
 							refs.Add(ref, uses)
-						case strings.Contains(uses.Value, "docker://"):
-							ref := resolver.NormalizeContainerRef(uses.Value)
-							refs.Add(ref, uses)
 						}
 					}
 				}
diff --git a/parser/actions_test.go b/parser/actions_test.go
index 94c56b0dfc..c1568a1140 100644
--- a/parser/actions_test.go
+++ b/parser/actions_test.go
@@ -28,6 +28,7 @@ jobs:
     steps:
       - uses: 'actions/checkout@v3'
       - uses: 'docker://ubuntu:20.04'
+      - uses: 'docker://ubuntu@sha256:47f14534bda344d9fe6ffd6effb95eefe579f4be0d508b7445cf77f61a0e5724'
         with:
           uses: 'foo/bar@v0'
   other_job:
@@ -39,6 +40,7 @@ jobs:
 				"actions://actions/checkout@v3",
 				"actions://org/repo/.github/workflows/other@v0",
 				"container://ubuntu:20.04",
+				"container://ubuntu@sha256:47f14534bda344d9fe6ffd6effb95eefe579f4be0d508b7445cf77f61a0e5724",
 			},
 		},
 		{
diff --git a/parser/parser.go b/parser/parser.go
index 8f9df8f8fe..d9464b141c 100644
--- a/parser/parser.go
+++ b/parser/parser.go
@@ -96,6 +96,14 @@ func Pin(ctx context.Context, res resolver.Resolver, parser Parser, m *yaml.Node
 	}
 	refs := refsList.All()
 
+	// Remove any absolute references from the list. We do not want to pin
+	// absolute references since they are already pinned.
+	for ref := range refs {
+		if isAbsolute(ref) {
+			delete(refs, ref)
+		}
+	}
+
 	sem := semaphore.NewWeighted(concurrency)
 
 	var merrLock sync.Mutex
diff --git a/parser/parser_test.go b/parser/parser_test.go
index bc3aec8f13..787a21d24a 100644
--- a/parser/parser_test.go
+++ b/parser/parser_test.go
@@ -87,7 +87,15 @@ func TestPin(t *testing.T) {
 	ctx := context.Background()
 
 	res, err := resolver.NewTest(map[string]*resolver.TestResult{
-		"actions://good/repo@v0": {Resolved: "good/repo@a12a3943"},
+		"actions://good/repo@v0": {
+			Resolved: "good/repo@a12a3943",
+		},
+		"actions://good/repo@2541b1294d2704b0964813337f33b291d3f8596b": {
+			Resolved: "good/repo@2541b1294d2704b0964813337f33b291d3f8596b",
+		},
+		"container://ubuntu@sha256:47f14534bda344d9fe6ffd6effb95eefe579f4be0d508b7445cf77f61a0e5724": {
+			Resolved: "ubuntu@sha256:47f14534bda344d9fe6ffd6effb95eefe579f4be0d508b7445cf77f61a0e5724",
+		},
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -138,6 +146,23 @@ jobs:
   my_job:
     steps:
       - uses: 'good/repo@a12a3943' # this is a comment ratchet:good/repo@v0
+`,
+		},
+		{
+			name: "already_pinned",
+			in: `
+jobs:
+  my_job:
+    steps:
+      - uses: 'good/repo@2541b1294d2704b0964813337f33b291d3f8596b' # ratchet:good/repo@v0
+      - uses: 'docker://ubuntu@sha256:47f14534bda344d9fe6ffd6effb95eefe579f4be0d508b7445cf77f61a0e5724' # ratchet:docker://ubuntu:20.04
+`,
+			exp: `
+jobs:
+  my_job:
+    steps:
+      - uses: 'good/repo@2541b1294d2704b0964813337f33b291d3f8596b' # ratchet:good/repo@v0
+      - uses: 'docker://ubuntu@sha256:47f14534bda344d9fe6ffd6effb95eefe579f4be0d508b7445cf77f61a0e5724' # ratchet:docker://ubuntu:20.04
 `,
 		},
 		{