Skip to content

Commit 2648bec

Browse files
katiexzhangkatiexzhang
authored
fix: Update PrintNamespacesList to use a YAML parser for multiple manifest files (#9825)
* Update PrintNamespacesList to break on YAML file separator '---' followed by a new line. * fix: Update PrintNamespacesList to break on YAML file separator '---' followed by a new line. * fix: update yaml line break character and add more tests * fix: use yaml parser to process multiple files * fix: add test for single manifest w break at end * fix: remove unnecessary conversion * fix: update import * fix: if resourceToInfoMap is empty, return nil * fix: format * fix: log fmt write error and update empty manifest (with just ---) handling * fix: format * fix: fix imports * fix: add test case
1 parent 537cb9b commit 2648bec

File tree

2 files changed

+214
-14
lines changed

2 files changed

+214
-14
lines changed

‎pkg/skaffold/inspect/namespaces/list.go‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,14 @@ limitations under the License.
1717
package inspect
1818

1919
import (
20+
"bytes"
2021
"context"
22+
"errors"
2123
"io"
2224
"log"
2325
"os"
24-
"strings"
2526

27+
"gopkg.in/yaml.v3"
2628
appsv1 "k8s.io/api/apps/v1"
2729
"k8s.io/kubectl/pkg/scheme"
2830

@@ -47,23 +49,39 @@ func PrintNamespacesList(ctx context.Context, out io.Writer, manifestFile string
4749
return err
4850
}
4951

52+
// Create a YAML decoder to handle multiple documents.
53+
yamlDecoder := yaml.NewDecoder(bytes.NewReader(b))
54+
5055
// Create a runtime.Decoder from the Codecs field within
5156
// k8s.io/client-go that's pre-loaded with the schemas for all
5257
// the standard Kubernetes resource types.
53-
decoder := scheme.Codecs.UniversalDeserializer()
58+
k8sDecoder := scheme.Codecs.UniversalDeserializer()
5459

5560
resourceToInfoMap := map[string][]resourceInfo{}
56-
for _, resourceYAML := range strings.Split(string(b), "---") {
61+
for {
62+
var value any
63+
err := yamlDecoder.Decode(&value)
64+
if errors.Is(err, io.EOF) {
65+
break
66+
}
67+
if err != nil {
68+
return err
69+
}
70+
valueBytes, err := yaml.Marshal(value)
71+
if err != nil {
72+
return err
73+
}
74+
5775
// skip empty documents, `Decode` will fail on them
58-
if len(resourceYAML) == 0 {
76+
if len(valueBytes) == 0 {
5977
continue
6078
}
6179
// - obj is the API object (e.g., Deployment)
6280
// - groupVersionKind is a generic object that allows
6381
// detecting the API type we are dealing with, for
6482
// accurate type casting later.
65-
obj, groupVersionKind, err := decoder.Decode(
66-
[]byte(resourceYAML),
83+
obj, groupVersionKind, err := k8sDecoder.Decode(
84+
valueBytes,
6785
nil,
6886
nil)
6987
if err != nil {
@@ -93,7 +111,10 @@ func PrintNamespacesList(ctx context.Context, out io.Writer, manifestFile string
93111
PropagateProfiles: opts.PropagateProfiles,
94112
})
95113
if err != nil {
96-
formatter.WriteErr(err)
114+
fmtErr := formatter.WriteErr(err)
115+
if fmtErr != nil {
116+
log.Print(fmtErr)
117+
}
97118
return err
98119
}
99120

‎pkg/skaffold/inspect/namespaces/list_test.go‎

Lines changed: 186 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,109 @@ spec:
5656
name: http
5757
`
5858

59-
var manifestWithNamespace = `apiVersion: apps/v1
59+
var manifestWithBreak = `apiVersion: apps/v1
60+
kind: Deployment
61+
metadata:
62+
labels:
63+
app: leeroy-app
64+
name: leeroy-app
65+
spec:
66+
replicas: 1
67+
selector:
68+
matchLabels:
69+
app: leeroy-app
70+
template:
71+
metadata:
72+
labels:
73+
app: leeroy-app
74+
spec:
75+
containers:
76+
- image: leeroy-app:1d38c165eada98acbbf9f8869b92bf32f4f9c4e80bdea23d20c7020db3ace2da
77+
name: leeroy-app
78+
ports:
79+
- containerPort: 50051
80+
name: http
81+
---
82+
`
83+
84+
var manifests = `apiVersion: apps/v1
85+
kind: Deployment
86+
metadata:
87+
labels:
88+
app: leeroy-app
89+
name: leeroy-app
90+
spec:
91+
replicas: 2
92+
selector:
93+
matchLabels:
94+
app: leeroy-app
95+
template:
96+
metadata:
97+
labels:
98+
app: leeroy-app
99+
spec:
100+
containers:
101+
- image: leeroy-app:1d38c165eada98acbbf9f8869b92bf32f4f9c4e80bdea23d20c7020db3ace2da
102+
name: leeroy-app
103+
ports:
104+
- containerPort: 50051
105+
name: http
106+
---
107+
apiVersion: apps/v1
108+
kind: Deployment
109+
metadata:
110+
labels:
111+
app: leeroy-app
112+
name: leeroy-app-canary
113+
spec:
114+
replicas: 1
115+
selector:
116+
matchLabels:
117+
app: leeroy-app
118+
template:
119+
metadata:
120+
labels:
121+
app: leeroy-app
122+
spec:
123+
containers:
124+
- image: leeroy-app2:1d38c165eada98acbbf9f8869b92bf32f4f9c4e80bdea23d20c7020db3ace2da
125+
name: leeroy-app
126+
ports:
127+
- containerPort: 50051
128+
name: http
129+
`
130+
131+
var manifestsWithNamespace = `apiVersion: apps/v1
60132
kind: Deployment
61133
metadata:
62134
labels:
63135
app: leeroy-app
64136
name: leeroy-app
65137
namespace: manifest-namespace
138+
spec:
139+
replicas: 2
140+
selector:
141+
matchLabels:
142+
app: leeroy-app
143+
template:
144+
metadata:
145+
labels:
146+
app: leeroy-app
147+
spec:
148+
containers:
149+
- image: leeroy-app:1d38c165eada98acbbf9f8869b92bf32f4f9c4e80bdea23d20c7020db3ace2da
150+
name: leeroy-app
151+
ports:
152+
- containerPort: 50051
153+
name: http
154+
---
155+
apiVersion: apps/v1
156+
kind: Deployment
157+
metadata:
158+
labels:
159+
app: leeroy-app
160+
name: leeroy-app-canary
161+
namespace: manifest-namespace
66162
spec:
67163
replicas: 1
68164
selector:
@@ -80,6 +176,40 @@ spec:
80176
- containerPort: 50051
81177
name: http
82178
`
179+
var manifestWithNamespace = `apiVersion: apps/v1
180+
kind: Deployment
181+
metadata:
182+
labels:
183+
app: leeroy-app
184+
name: leeroy-app---rel01
185+
namespace: manifest-namespace
186+
spec:
187+
replicas: 1
188+
selector:
189+
matchLabels:
190+
app: leeroy-app
191+
template:
192+
metadata:
193+
labels:
194+
app: leeroy-app
195+
spec:
196+
containers:
197+
- image: leeroy-app:1d38c165eada98acbbf9f8869b92bf32f4f9c4e80bdea23d20c7020db3ace2da
198+
name: leeroy-app
199+
ports:
200+
- containerPort: 50051
201+
name: http
202+
`
203+
204+
var emptyManifestWithBreak = `
205+
---
206+
`
207+
208+
var emptyManifestWithTwoBreaks = `
209+
---
210+
211+
---
212+
`
83213

84214
func TestPrintTestsList(t *testing.T) {
85215
tests := []struct {
@@ -91,38 +221,87 @@ func TestPrintTestsList(t *testing.T) {
91221
expected string
92222
}{
93223
{
94-
description: "print all deployment namespaces where no namespace is set in manifest(s) or deploy config",
224+
description: "no namespace set in manifest or deploy config",
95225
manifest: manifest,
96226
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"default"}]}}` + "\n",
97227
module: []string{"cfg-without-default-namespace"},
98228
},
99229
{
100-
description: "print all deployment namespaces where a namespace is set via the kubectl flag deploy config",
230+
description: "no namespace set in manifest with break or deploy config",
231+
manifest: manifestWithBreak,
232+
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"default"}]}}` + "\n",
233+
module: []string{"cfg-without-default-namespace"},
234+
},
235+
{
236+
description: "no namespace is set in manifests or deploy config",
237+
manifest: manifests,
238+
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"default"},{"name":"leeroy-app-canary","namespace":"default"}]}}` + "\n",
239+
module: []string{"cfg-without-default-namespace"},
240+
},
241+
{
242+
description: "namespace set via kubectl flag deploy config for single manifest",
101243
manifest: manifest,
102244
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"foo-flag-ns"}]}}` + "\n",
103245
profiles: []string{"foo-flag-ns"},
104246
module: []string{"cfg-without-default-namespace"},
105247
},
106248
{
107-
description: "print all deployment namespaces where a default namespace is set via the kubectl defaultNamespace deploy config",
249+
description: "namespace set via kubectl flag deploy config for multiple manifests",
250+
manifest: manifests,
251+
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"foo-flag-ns"},{"name":"leeroy-app-canary","namespace":"foo-flag-ns"}]}}` + "\n",
252+
profiles: []string{"foo-flag-ns"},
253+
module: []string{"cfg-without-default-namespace"},
254+
},
255+
{
256+
description: "default namespace set via the kubectl defaultNamespace deploy config for single manifest",
108257
manifest: manifest,
109258
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"bar"}]}}` + "\n",
110259
module: []string{"cfg-with-default-namespace"},
111260
},
112261
{
113-
description: "print all deployment namespaces where a default namespace and namespace is set via the kubectl deploy config",
262+
description: "default namespace set via the kubectl defaultNamespace deploy config for multiple manifests",
263+
manifest: manifests,
264+
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"bar"},{"name":"leeroy-app-canary","namespace":"bar"}]}}` + "\n",
265+
module: []string{"cfg-with-default-namespace"},
266+
},
267+
{
268+
description: "default namespace and namespace set via the kubectl deploy config for single manifest",
114269
manifest: manifest,
115270
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"baz-flag-ns"}]}}` + "\n",
116271
profiles: []string{"baz-flag-ns"},
117272
module: []string{"cfg-with-default-namespace"},
118273
},
119274
{
120-
description: "print all deployment namespaces where the manifest has a namespace set but it is also set via the kubectl flag deploy config",
275+
description: "default namespace and namespace set via the kubectl deploy config for multiple manifests",
276+
manifest: manifests,
277+
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"baz-flag-ns"},{"name":"leeroy-app-canary","namespace":"baz-flag-ns"}]}}` + "\n",
278+
profiles: []string{"baz-flag-ns"},
279+
module: []string{"cfg-with-default-namespace"},
280+
},
281+
{
282+
description: "manifest has namespace set and namespace also set via kubectl flag deploy config",
121283
manifest: manifestWithNamespace,
122-
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"manifest-namespace"}]}}` + "\n",
284+
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app---rel01","namespace":"manifest-namespace"}]}}` + "\n",
123285
profiles: []string{"baz-flag-ns"},
124286
module: []string{"cfg-with-default-namespace"},
125287
},
288+
{
289+
description: "manifests have namespace set and namespace also set via the kubectl flag deploy config",
290+
manifest: manifestsWithNamespace,
291+
expected: `{"resourceToInfoMap":{"apps/v1, Kind=Deployment":[{"name":"leeroy-app","namespace":"manifest-namespace"},{"name":"leeroy-app-canary","namespace":"manifest-namespace"}]}}` + "\n",
292+
profiles: []string{"baz-flag-ns"},
293+
module: []string{"cfg-with-default-namespace"},
294+
},
295+
{
296+
description: "empty manifest with yaml page break notation returns empty resourceToInfoMap",
297+
manifest: emptyManifestWithBreak,
298+
expected: `{"resourceToInfoMap":{}}` + "\n",
299+
},
300+
{
301+
description: "empty manifest with more than one yaml page break notation returns empty resourceToInfoMap",
302+
manifest: emptyManifestWithTwoBreaks,
303+
expected: `{"resourceToInfoMap":{}}` + "\n",
304+
},
126305
{
127306
description: "actionable error",
128307
manifest: manifest,

0 commit comments

Comments
 (0)