Ensure pinning is idempotent (#22)

Previously, running "pin" on an already-pinned file would lose the original ratchet references.
sethvargo · May 27, 2022 · bc83ec3 · bc83ec3
1 parent d3b4f23
commit bc83ec3
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 11 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
 `,
 		},
 		{