Skip to content

Commit 52a5153

Browse files
weikanglimCopilot
andcommitted
test: address PR feedback for layer hooks
Remove the local schema directive from the hooks sample, reject duplicate layer names and mutually exclusive --service/--layer usage, and clean up schema definitions used by docs generation. Keep the HOOK_TRACE_FILE env handling unchanged and document the Go exec.Cmd.Env behavior in PR discussion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2ee8f02 commit 52a5153

8 files changed

Lines changed: 111 additions & 6 deletions

File tree

cli/azd/cmd/hooks.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,17 @@ var knownHookNames = map[string]bool{
145145
func (hra *hooksRunAction) Run(ctx context.Context) (*actions.ActionResult, error) {
146146
hookName := hra.args[0]
147147

148+
if hra.flags.service != "" && hra.flags.layer != "" {
149+
return nil, &internal.ErrorWithSuggestion{
150+
Err: fmt.Errorf("--service and --layer cannot be used together"),
151+
Suggestion: "Choose either '--service' to run service hooks or '--layer' to run provisioning layer hooks.",
152+
}
153+
}
154+
148155
hookType := "project"
149-
if hra.flags.service != "" {
156+
if hra.flags.layer != "" {
157+
hookType = "layer"
158+
} else if hra.flags.service != "" {
150159
hookType = "service"
151160
}
152161

cli/azd/cmd/hooks_test.go

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"path/filepath"
99
"testing"
1010

11+
"github.com/azure/azure-dev/cli/azd/internal/tracing"
12+
"github.com/azure/azure-dev/cli/azd/internal/tracing/fields"
1113
"github.com/azure/azure-dev/cli/azd/pkg/environment"
1214
"github.com/azure/azure-dev/cli/azd/pkg/exec"
1315
"github.com/azure/azure-dev/cli/azd/pkg/ext"
@@ -152,3 +154,76 @@ func Test_HooksRunAction_FiltersLayerHooks(t *testing.T) {
152154
filepath.Join(projectPath, "infra/shared"),
153155
}, gotCwds)
154156
}
157+
158+
func Test_HooksRunAction_SetsTelemetryTypeForLayer(t *testing.T) {
159+
mockContext := mocks.NewMockContext(context.Background())
160+
env := environment.NewWithValues("test", nil)
161+
envManager := &mockenv.MockEnvManager{}
162+
envManager.On("Reload", mock.Anything, mock.Anything).Return(nil)
163+
164+
t.Cleanup(func() {
165+
tracing.SetUsageAttributes()
166+
})
167+
tracing.SetUsageAttributes()
168+
169+
projectConfig := &project.ProjectConfig{
170+
Name: "test",
171+
Path: t.TempDir(),
172+
Services: map[string]*project.ServiceConfig{},
173+
Infra: provisioning.Options{
174+
Layers: []provisioning.Options{
175+
{
176+
Name: "core",
177+
Path: "infra/core",
178+
Hooks: provisioning.HooksConfig{
179+
"preprovision": {{
180+
Shell: ext.ShellTypeBash,
181+
Run: "echo core",
182+
}},
183+
},
184+
},
185+
},
186+
},
187+
}
188+
189+
mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool {
190+
return true
191+
}).Respond(exec.NewRunResult(0, "", ""))
192+
193+
action := &hooksRunAction{
194+
projectConfig: projectConfig,
195+
env: env,
196+
envManager: envManager,
197+
importManager: project.NewImportManager(nil),
198+
commandRunner: mockContext.CommandRunner,
199+
console: mockContext.Console,
200+
flags: &hooksRunFlags{layer: "core"},
201+
args: []string{"preprovision"},
202+
serviceLocator: mockContext.Container,
203+
}
204+
205+
_, err := action.Run(*mockContext.Context)
206+
require.NoError(t, err)
207+
208+
var hookType string
209+
for _, attr := range tracing.GetUsageAttributes() {
210+
if attr.Key == fields.HooksTypeKey.Key {
211+
hookType = attr.Value.AsString()
212+
break
213+
}
214+
}
215+
216+
require.Equal(t, "layer", hookType)
217+
}
218+
219+
func Test_HooksRunAction_RejectsServiceAndLayerTogether(t *testing.T) {
220+
action := &hooksRunAction{
221+
env: environment.NewWithValues("test", nil),
222+
flags: &hooksRunFlags{service: "api", layer: "core"},
223+
args: []string{"preprovision"},
224+
}
225+
226+
_, err := action.Run(context.Background())
227+
require.Error(t, err)
228+
require.ErrorContains(t, err, "--service and --layer cannot be used together")
229+
}

cli/azd/internal/tracing/fields/fields.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ var (
345345
Classification: SystemMetadata,
346346
Purpose: FeatureInsight,
347347
}
348-
// The type of the hook (project or service).
348+
// The type of the hook run scope (project, layer, or service).
349349
HooksTypeKey = AttributeKey{
350350
Key: attribute.Key("hooks.type"),
351351
Classification: SystemMetadata,

cli/azd/pkg/infra/provisioning/provider.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,11 +161,18 @@ func (o *Options) Validate() error {
161161
"properties on 'infra' cannot be declared when 'infra.layers' is declared")
162162
}
163163

164+
seenLayers := map[string]struct{}{}
164165
for _, layer := range o.Layers {
165166
if layer.Name == "" {
166167
return errWrap("name must be specified for each provisioning layer")
167168
}
168169

170+
if _, has := seenLayers[layer.Name]; has {
171+
return errWrap(fmt.Sprintf("duplicate layer name '%s' is not allowed", layer.Name))
172+
}
173+
174+
seenLayers[layer.Name] = struct{}{}
175+
169176
if layer.Path == "" {
170177
return errWrap(fmt.Sprintf("%s: path must be specified", layer.Name))
171178
}

cli/azd/pkg/infra/provisioning/provider_test.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -429,6 +429,24 @@ func TestOptions_Validate_Hooks(t *testing.T) {
429429
require.ErrorContains(t, err, "only 'preprovision' and 'postprovision' hooks are supported")
430430
})
431431

432+
t.Run("duplicate layer names are not allowed", func(t *testing.T) {
433+
err := (&Options{
434+
Layers: []Options{
435+
{
436+
Name: "infra-core",
437+
Path: "infra/core",
438+
},
439+
{
440+
Name: "infra-core",
441+
Path: "infra/shared",
442+
},
443+
},
444+
}).Validate()
445+
446+
require.Error(t, err)
447+
require.ErrorContains(t, err, "duplicate layer name 'infra-core' is not allowed")
448+
})
449+
432450
t.Run("layers cannot be mixed with root hooks", func(t *testing.T) {
433451
err := (&Options{
434452
Path: "infra",

cli/azd/test/functional/testdata/samples/hooks/azure.yaml

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
# yaml-language-server: $schema=/Users/weilim/repos/azd2/schemas/v1.0/azure.yaml.json
2-
31
name: hooks
42

53
infra:

schemas/alpha/azure.yaml.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@
6767
"type": "array",
6868
"title": "Provisioning layers.",
6969
"description": "Optional. Layers for Azure infrastructure provisioning.",
70-
"additionalProperties": false,
7170
"uniqueItems": true,
7271
"items": {
7372
"type": "object",

schemas/v1.0/azure.yaml.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@
6464
"type": "array",
6565
"title": "Provisioning layers.",
6666
"description": "Optional. Layers for Azure infrastructure provisioning.",
67-
"additionalProperties": false,
6867
"uniqueItems": true,
6968
"items": {
7069
"type": "object",

0 commit comments

Comments
 (0)