Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support azd hooks out of azure.yaml, Part I of merging azd operation to hooks #4244

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Support azd hooks out of azure.yaml, Part I of merging azd operation to hooks #4244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4d59c6d
service hooks
vhvb1989 Aug 23, 2024
fc2b714
Merge branch 'main' of github.com:Azure/azure-dev into merge-azd-oper…
vhvb1989 Aug 23, 2024
3c3b954
As part of merging azd operation and hooks, this PR allows users to d…
vhvb1989 Aug 23, 2024
9a561f5
test new function
vhvb1989 Aug 24, 2024
704c5d1
Merge branch 'main' of github.com:Azure/azure-dev into merge-azd-oper…
vhvb1989 Aug 24, 2024
0c6cdf4
Merge branch 'main' of github.com:Azure/azure-dev into merge-azd-oper…
vhvb1989 Aug 26, 2024
a65c3b4
lint
vhvb1989 Aug 26, 2024
8605e02
Merge branch 'main' of github.com:Azure/azure-dev into merge-azd-oper…
vhvb1989 Aug 29, 2024
af3da51
wait here
vhvb1989 Aug 29, 2024
99ca56a
Merge branch 'main' of github.com:Azure/azure-dev into merge-azd-oper…
vhvb1989 Aug 30, 2024
a292068
User project to parse hooks from external files
vhvb1989 Aug 30, 2024
1178156
ignore all stat errors as a way to handle Aspire projects
vhvb1989 Aug 30, 2024
deae098
Merge branch 'main' of github.com:Azure/azure-dev into merge-azd-oper…
vhvb1989 Sep 5, 2024
5b2ae88
Merge branch 'main' of github.com:Azure/azure-dev into merge-azd-oper…
vhvb1989 Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 66 additions & 2 deletions cli/azd/pkg/project/project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,24 @@ func Load(ctx context.Context, projectFilePath string) (*ProjectConfig, error) {
return nil, fmt.Errorf("parsing project file: %w", err)
}

projectConfig.Path = filepath.Dir(projectFilePath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't put the hooks in /infra folder to make the not service specific.

If you put in the infra folder then that means they are related to infra, which they can be, but that isn't always true.

I recommend either:

  1. Put them in the root
  2. Create a new folder, something like hooks, or scripts/hooks.

Copy link
Contributor

@wbreza wbreza Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% agree here. The hooks config file should either be in the same folder as the azure.yaml for project level command hooks or in a project folder of a service for service specific hooks.

Only provision hooks are related to infra and that shouldn't override configuration for all hook configuration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding it to /infra is the fundamental intention of this PR, :)
azd.operations are infrastructure-extensions.
When translating to azd.operations to hooks, we would expect the azd.hooks in the /infra folder to use either postprovision or preprovision. Otherwise, hooks would be defined in azure.yaml.

I don't expect folks to migrate from using azure.yaml to use azd.hooks file in /infra folder. The intention is to allow projects like Aspire to generate the infrastructure including hooks (currently known as azd.operations).

Would you feel better if azd fails when using azd.hooks in /infra to define a non postprovision or preprovision hook?

However, I wouldn't find too much value in such validation.

The expectation, when using Aspire to generate infrastructure (with azd infra synth) is that folks can easily delete the infra (by deleing the infra folder) to go back to auto-in-memory-generation. If the azd.hooks file goes out of /infra, we would be adding an extra step for cleaning the infra files.

Copy link
Member

@jongio jongio Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about updating this to work with any file in the project that is prefixed with azd.hooks, but not make /infra the standard. As-in, if the user Really wants to put it in infra. We don't stop them, but if they want to put it in the route, they can. They can put it wherever they want in your particular case. You want it in infra and that should be fine with a flexible design, But I don't think we should force them to put it in infra folder.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use this PR as the first step on that direction? Then wait and measure usage and if customers ask for more?

My expectation is that customers will continue to use azure.yaml for hooks, as you have the yaml schema to help you use IntelliSense and spot errors. The azd.hooks file is coming right now as an alternative for code-generators.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The azd operations is an alpha feature and is subject to change. As we discussed these really are the same as hooks with more hard coded operations vs generic scripts. Because of this I would not want to see azd operations continue forward as its own unique feature for only infra related activities. For aspire scenarios it really shouldn't matter whether you generate this file in the infra folder or in the project root.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For aspire scenarios it really shouldn't matter whether you generate this file in the infra folder or in the project root.

It matters a lot!! For customers running azd infra synth, the output goes to the /infra folder.
We used to output to /infra folder + services' folders and got user's feedback/request to consolidate it all in one single folder (so they could remove it if they want to go back to Aspire-generation).

That's why it is fundamental in this case to start by supporting the azd.hooks file at /infra folder, to solve the main user-case we have right now.
In the future, we can expand to all folders if we want (or customers ask for it). Right now, I think it would be much more than what we need.

And yes, after this, azd operations alpha feature would be retired :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the infra gets generated in the infra folder but this doesn't mean and hooks configuration should be here also. I'd strongly advise we don't include the hooks yaml in the infra and move this to the root as suggested above.


// complement the project config with azd.hooks files if they exist
hooksDefinedAtInfraPath, err := hooksFromFolderPath(filepath.Join(projectConfig.Path, projectConfig.Infra.Path))
if err != nil {
return nil, fmt.Errorf("failed getting hooks from infra path, %w", err)
}
if len(hooksDefinedAtInfraPath) > 0 && len(projectConfig.Hooks) > 0 {
return nil, fmt.Errorf(
"project hooks defined in both %s and azure.yaml configuration,"+
" please remove one of them",
filepath.Join(projectConfig.Infra.Path, "azd.hooks.yaml"),
)
}
if projectConfig.Hooks == nil {
projectConfig.Hooks = hooksDefinedAtInfraPath
}

if projectConfig.Metadata != nil && projectConfig.Metadata.Template != "" {
template := strings.Split(projectConfig.Metadata.Template, "@")
if len(template) == 1 { // no version specifier, just the template ID
Expand All @@ -177,6 +195,20 @@ func Load(ctx context.Context, projectFilePath string) (*ProjectConfig, error) {
for _, svcConfig := range projectConfig.Services {
hosts[i] = string(svcConfig.Host)
languages[i] = string(svcConfig.Language)

// complement service level hooks
hooksDefinedAtServicePath, err := hooksFromFolderPath(svcConfig.RelativePath)
if err != nil {
return nil, err
}
if svcConfig.Hooks != nil && hooksDefinedAtServicePath != nil {
return nil, fmt.Errorf("service %s has hooks defined in both azd.hooks.yaml and azure.yaml, "+
"please remove one of them.", svcConfig.Name)
}
if svcConfig.Hooks == nil {
svcConfig.Hooks = hooksDefinedAtServicePath
}

i++
}

Expand All @@ -187,7 +219,6 @@ func Load(ctx context.Context, projectFilePath string) (*ProjectConfig, error) {
tracing.SetUsageAttributes(fields.ProjectServiceHostsKey.StringSlice(hosts))
}

projectConfig.Path = filepath.Dir(projectFilePath)
return projectConfig, nil
}

Expand Down Expand Up @@ -263,7 +294,40 @@ func Save(ctx context.Context, projectConfig *ProjectConfig, projectFilePath str
return fmt.Errorf("saving project file: %w", err)
}

projectConfig.Path = projectFilePath
projectConfig.Path = filepath.Dir(projectFilePath)

return nil
}

// hooksFromFolderPath check if there is file named azd.hooks.yaml in the service path
// and return the hooks configuration.
func hooksFromFolderPath(servicePath string) (HooksConfig, error) {
hooksPath := filepath.Join(servicePath, "azd.hooks.yaml")

// due to projects depending on a ProjectImporter like Aspire, we need to ignore all type of errors related to
// the path is either not found, not accessible or any other error that might occur.
// In case of Aspire, the servicePath points out to the C# project, and not to the directory.
// We could handle Aspire Project here but that's not the purpose of this function.
// The right thing might be to use the ProjectImporter and get the list of services from Aspire and check for hooks at
// each path, but hooks for Aspire services are not even supported on azure.yaml.
if _, err := os.Stat(hooksPath); err != nil {
hooksPath = filepath.Join(servicePath, "azd.hooks.yml")
if _, err := os.Stat(hooksPath); err != nil {
log.Println("error trying to read hooks file for service in path: ", servicePath, ". Error:", err)
return nil, nil
}
}

hooksFile, err := os.ReadFile(hooksPath)
if err != nil {
return nil, fmt.Errorf("failed reading hooks from '%s', %w", hooksPath, err)
}

// open hooksPath into a byte array and unmarshal it into a map[string]*ext.HookConfig
hooks := make(HooksConfig)
if err := yaml.Unmarshal(hooksFile, &hooks); err != nil {
return nil, fmt.Errorf("failed unmarshalling hooks from '%s', %w", hooksPath, err)
}

return hooks, nil
}
214 changes: 214 additions & 0 deletions cli/azd/pkg/project/project_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package project

import (
"context"
"os"
"path/filepath"
"testing"

Expand All @@ -13,7 +14,9 @@ import (
"github.com/azure/azure-dev/cli/azd/pkg/azapi"
"github.com/azure/azure-dev/cli/azd/pkg/azure"
"github.com/azure/azure-dev/cli/azd/pkg/environment"
"github.com/azure/azure-dev/cli/azd/pkg/ext"
"github.com/azure/azure-dev/cli/azd/pkg/infra"
"github.com/azure/azure-dev/cli/azd/pkg/osutil"
"github.com/azure/azure-dev/cli/azd/test/mocks"
"github.com/azure/azure-dev/cli/azd/test/mocks/mockarmresources"
"github.com/azure/azure-dev/cli/azd/test/mocks/mockazcli"
Expand Down Expand Up @@ -342,3 +345,214 @@ services:
assert.Equal(t, filepath.FromSlash("src/api"), projectConfig.Services["api"].RelativePath)
assert.Equal(t, filepath.FromSlash("bin/api"), projectConfig.Services["api"].OutputPath)
}

func Test_HooksFromFolderPath(t *testing.T) {
t.Run("ProjectInfraHooks", func(t *testing.T) {
prj := &ProjectConfig{
Name: "minimal",
Services: map[string]*ServiceConfig{},
}
contents, err := yaml.Marshal(prj)
require.NoError(t, err)

tempDir := t.TempDir()

azureYamlPath := filepath.Join(tempDir, "azure.yaml")
err = os.WriteFile(azureYamlPath, contents, osutil.PermissionDirectory)
require.NoError(t, err)

infraPath := filepath.Join(tempDir, "infra")
err = os.Mkdir(infraPath, osutil.PermissionDirectory)
require.NoError(t, err)

hooksPath := filepath.Join(infraPath, "azd.hooks.yaml")
hooksContent := []byte(`
pre-build:
shell: sh
run: ./pre-build.sh
post-build:
shell: pwsh
run: ./post-build.ps1
`)

err = os.WriteFile(hooksPath, hooksContent, osutil.PermissionDirectory)
require.NoError(t, err)

expectedHooks := HooksConfig{
"pre-build": {{
Name: "",
Shell: ext.ShellTypeBash,
Run: "./pre-build.sh",
ContinueOnError: false,
Interactive: false,
Windows: nil,
Posix: nil,
}},
"post-build": {{
Name: "",
Shell: ext.ShellTypePowershell,
Run: "./post-build.ps1",
ContinueOnError: false,
Interactive: false,
Windows: nil,
Posix: nil,
},
}}

project, err := Load(context.Background(), azureYamlPath)
require.NoError(t, err)
require.Equal(t, expectedHooks, project.Hooks)
})

t.Run("ErrorDoubleDefintionHooks", func(t *testing.T) {
prj := &ProjectConfig{
Name: "minimal",
Services: map[string]*ServiceConfig{},
Hooks: HooksConfig{
"prebuild": {{
Run: "./pre-build.sh",
}},
},
}
contents, err := yaml.Marshal(prj)
require.NoError(t, err)

tempDir := t.TempDir()

azureYamlPath := filepath.Join(tempDir, "azure.yaml")
err = os.WriteFile(azureYamlPath, contents, osutil.PermissionDirectory)
require.NoError(t, err)

infraPath := filepath.Join(tempDir, "infra")
err = os.Mkdir(infraPath, osutil.PermissionDirectory)
require.NoError(t, err)

hooksPath := filepath.Join(infraPath, "azd.hooks.yaml")
hooksContent := []byte(`
pre-build:
shell: sh
run: ./pre-build.sh
post-build:
shell: pwsh
run: ./post-build.ps1
`)

err = os.WriteFile(hooksPath, hooksContent, osutil.PermissionDirectory)
require.NoError(t, err)

project, err := Load(context.Background(), azureYamlPath)
require.Error(t, err)
var expectedProject *ProjectConfig
require.Equal(t, expectedProject, project)
})

t.Run("ServiceInfraHooks", func(t *testing.T) {
tempDir := t.TempDir()

prj := &ProjectConfig{
Name: "minimal",
Services: map[string]*ServiceConfig{
"api": {
Name: "api",
Host: AppServiceTarget,
RelativePath: filepath.Join(tempDir, "api"),
},
},
}
contents, err := yaml.Marshal(prj)
require.NoError(t, err)

azureYamlPath := filepath.Join(tempDir, "azure.yaml")
err = os.WriteFile(azureYamlPath, contents, osutil.PermissionDirectory)
require.NoError(t, err)

servicePath := filepath.Join(tempDir, "api")
err = os.Mkdir(servicePath, osutil.PermissionDirectory)
require.NoError(t, err)

hooksPath := filepath.Join(servicePath, "azd.hooks.yaml")
hooksContent := []byte(`
pre-build:
shell: sh
run: ./pre-build.sh
post-build:
shell: pwsh
run: ./post-build.ps1
`)

err = os.WriteFile(hooksPath, hooksContent, osutil.PermissionDirectory)
require.NoError(t, err)

expectedHooks := HooksConfig{
"pre-build": {{
Name: "",
Shell: ext.ShellTypeBash,
Run: "./pre-build.sh",
ContinueOnError: false,
Interactive: false,
Windows: nil,
Posix: nil,
}},
"post-build": {{
Name: "",
Shell: ext.ShellTypePowershell,
Run: "./post-build.ps1",
ContinueOnError: false,
Interactive: false,
Windows: nil,
Posix: nil,
},
}}

project, err := Load(context.Background(), azureYamlPath)
require.NoError(t, err)
require.Equal(t, expectedHooks, project.Services["api"].Hooks)
})

t.Run("ErrorDoubleDefintionServiceHooks", func(t *testing.T) {
tempDir := t.TempDir()
prj := &ProjectConfig{
Name: "minimal",
Services: map[string]*ServiceConfig{
"api": {
Name: "api",
Host: AppServiceTarget,
Hooks: HooksConfig{
"prebuild": {{
Run: "./pre-build.sh",
}},
},
RelativePath: filepath.Join(tempDir, "api"),
},
},
}
contents, err := yaml.Marshal(prj)
require.NoError(t, err)

azureYamlPath := filepath.Join(tempDir, "azure.yaml")
err = os.WriteFile(azureYamlPath, contents, osutil.PermissionDirectory)
require.NoError(t, err)

servicePath := filepath.Join(tempDir, "api")
err = os.Mkdir(servicePath, osutil.PermissionDirectory)
require.NoError(t, err)

hooksPath := filepath.Join(servicePath, "azd.hooks.yaml")
hooksContent := []byte(`
pre-build:
shell: sh
run: ./pre-build.sh
post-build:
shell: pwsh
run: ./post-build.ps1
`)

err = os.WriteFile(hooksPath, hooksContent, osutil.PermissionDirectory)
require.NoError(t, err)

project, err := Load(context.Background(), azureYamlPath)
require.Error(t, err)
var expectedProject *ProjectConfig
require.Equal(t, expectedProject, project)
})
}
Loading