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.

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

Add .webappignore and .funcignore Support for Service Packaging #4258

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions .vscode/cspell.global.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ ignoreWords:
- azurewebsites
- azcore
- Azdo
- azdignore
- azdrelease
- aztfmod
- azurecaf
Expand All @@ -59,9 +60,11 @@ ignoreWords:
- dapr
- databricks
- dedb
- denormal
- devcenter
- devcontainer
- dnsz
- dotignore
- evgd
- evgs
- evgt
Expand All @@ -71,6 +74,8 @@ ignoreWords:
- fdfp
- fics
- Frontdoor
- funcignore
- gitcli
- golobby
- graphsdk
- hndl
Expand Down Expand Up @@ -158,6 +163,7 @@ ignoreWords:
- vwan
- wafrg
- westus
- webappignore
- Wans
- apim
- Retryable
Expand All @@ -176,7 +182,9 @@ ignoreWords:
- venv
- webapprouting
- zipdeploy
- zipignore
- appinsights
- pycache
useGitignore: true
dictionaryDefinitions:
- name: gitHubUserAliases
Expand Down
2 changes: 1 addition & 1 deletion cli/azd/.vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,4 @@
"default": "auth login --use-device-code"
}
]
}
}
44 changes: 44 additions & 0 deletions cli/azd/pkg/dotignore/dotignore.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package dotignore

import (
"fmt"
"os"
"path/filepath"

"github.com/denormal/go-gitignore"
)

// ReadDotIgnoreFile reads the ignore file located at the root of the project directory.
// If the ignoreFileName is blank or the file is not found, it returns nil, nil.
func ReadDotIgnoreFile(projectDir string, ignoreFileName string) (gitignore.GitIgnore, error) {
// Return nil if the ignoreFileName is empty
if ignoreFileName == "" {
return nil, nil
}

ignoreFilePath := filepath.Join(projectDir, ignoreFileName)
if _, err := os.Stat(ignoreFilePath); os.IsNotExist(err) {
// Return nil if the ignore file does not exist
return nil, nil
}

ignoreMatcher, err := gitignore.NewFromFile(ignoreFilePath)
if err != nil {
return nil, fmt.Errorf("error reading %s file at %s: %w", ignoreFileName, ignoreFilePath, err)
}

return ignoreMatcher, nil
}

// ShouldIgnore determines whether a file or directory should be ignored based on the provided ignore matcher.
func ShouldIgnore(relativePath string, isDir bool, ignoreMatcher gitignore.GitIgnore) bool {
if ignoreMatcher != nil {
if match := ignoreMatcher.Relative(relativePath, isDir); match != nil && match.Ignore() {
return true
}
}
return false
}
8 changes: 4 additions & 4 deletions cli/azd/pkg/project/framework_service_npm.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,10 @@ func (np *npmProject) Package(
packageSource,
packageDest,
buildForZipOptions{
excludeConditions: []excludeDirEntryCondition{
excludeNodeModules,
},
}); err != nil {
excludeConditions: []excludeDirEntryCondition{excludeNodeModules},
},
serviceConfig,
); err != nil {
return nil, fmt.Errorf("packaging for %s: %w", serviceConfig.Name, err)
}

Expand Down
9 changes: 4 additions & 5 deletions cli/azd/pkg/project/framework_service_python.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,10 @@ func (pp *pythonProject) Package(
packageSource,
packageDest,
buildForZipOptions{
excludeConditions: []excludeDirEntryCondition{
excludeVirtualEnv,
excludePyCache,
},
}); err != nil {
excludeConditions: []excludeDirEntryCondition{excludeVirtualEnv, excludePyCache},
},
serviceConfig,
); err != nil {

return nil, fmt.Errorf("packaging for %s: %w", serviceConfig.Name, err)
}
Expand Down
54 changes: 41 additions & 13 deletions cli/azd/pkg/project/project_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,34 +9,36 @@ import (
"path/filepath"
"time"

"github.com/azure/azure-dev/cli/azd/pkg/dotignore"
"github.com/azure/azure-dev/cli/azd/pkg/rzip"
"github.com/otiai10/copy"
)

// CreateDeployableZip creates a zip file of a folder, recursively.
// createDeployableZip creates a zip file of a folder, recursively.
// Returns the path to the created zip file or an error if it fails.
func createDeployableZip(projectName string, appName string, path string) (string, error) {
// TODO: should probably avoid picking up files that weren't meant to be deployed (ie, local .env files, etc..)
// Create the output zip file path
filePath := filepath.Join(os.TempDir(), fmt.Sprintf("%s-%s-azddeploy-%d.zip", projectName, appName, time.Now().Unix()))
zipFile, err := os.Create(filePath)
if err != nil {
return "", fmt.Errorf("failed when creating zip package to deploy %s: %w", appName, err)
}

if err := rzip.CreateFromDirectory(path, zipFile); err != nil {
// if we fail here just do our best to close things out and cleanup
// Zip the directory without any exclusions (they've already been handled in buildForZip)
err = rzip.CreateFromDirectory(path, zipFile)
if err != nil {
zipFile.Close()
os.Remove(zipFile.Name())
return "", err
}

// Close the zip file and return the path
if err := zipFile.Close(); err != nil {
// may fail but, again, we'll do our best to cleanup here.
os.Remove(zipFile.Name())
return "", err
}

return zipFile.Name(), nil
return filePath, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming filePath gives us the full absolute path while zipFile.Name() was only giving us the filename? I would just double check that this is honored downstream of the caller.

}

// excludeDirEntryCondition resolves when a file or directory should be considered or not as part of build, when build is a
Expand All @@ -48,17 +50,43 @@ type buildForZipOptions struct {
excludeConditions []excludeDirEntryCondition
}

// buildForZip is use by projects which build strategy is to only copy the source code into a folder which is later
// zipped for packaging. For example Python and Node framework languages. buildForZipOptions provides the specific
// details for each language which should not be ever copied.
func buildForZip(src, dst string, options buildForZipOptions) error {
// buildForZip is used by projects to prepare a directory for
// zipping, excluding files based on the ignore file and other conditions.
func buildForZip(src, dst string, options buildForZipOptions, serviceConfig *ServiceConfig) error {
// Lookup the appropriate ignore file name based on the service kind (Host)
ignoreFileName := GetIgnoreFileNameByKind(serviceConfig.Host)

// Read and honor the specified ignore file if it exists
ignoreMatcher, err := dotignore.ReadDotIgnoreFile(src, ignoreFileName)
Copy link
Contributor

@weikanglim weikanglim Sep 18, 2024

Choose a reason for hiding this comment

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

Are we also thinking about supporting *.ignore files in any recursive child directory of the service?

- funcapp/
   - .funcignore
   - src/
     - otherThing/
       - .funcignore # define ignore files relative to this path

In my mind, I think azd would honor both ignore files.

If we're doing this, I think we should consider implementing the correct recursion logic in project before generalizing it as a dotignore package -- I would like to see the design matured out before we push it up to an app library package in azd.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ellismg had implemented .dockerignore recently -- wonder if he has thoughts here.

Copy link
Member Author

@jongio jongio Sep 19, 2024

Choose a reason for hiding this comment

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

.funcignore only honors files in the service root and in discussion with them and the app service team we decided to not honor recursive .ignore files.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ellismg had implemented .dockerignore recently

@ellismg - Are you thinking we migrate the pack_source.go implementation to use the new dotignore support in this PR?

Copy link
Contributor

@weikanglim weikanglim Sep 19, 2024

Choose a reason for hiding this comment

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

.funcignore only honors files in the root and in discussion with them and the app service team we decided to not honor recursive .ignore files.

That is quite an interesting decision. From a dev perspective, one main reason why *.ignore files are used today is precisely because they can always be placed relative to a directory and provide a more local ignore scope. If that is not being done, a simpler package manifest at the root of the package directory would be a more ideal format IMO.

I took a closer look. It also turns out that .funcignore does not conform to the .gitignore spec entirely. @ellismg @jongio

  • The issue on functools
  • Based on the previous issue, I gather that the library they're using is gitignore-parser with its set of issues.

If we're already engaged in conversations with both app service and function teams, can we consider:

  1. Evaluating the impact of having two different .funcignore interpretations (is there user pain we would worry about?)
  2. Based on those conversations, decide if we would like to rally all teams to adopt the conformant spec, and think about potentially support recursive directories. We could also just support recursive directories from azd side for now, it would be perhaps 20 lines of code, and something users would care about -- perhaps not today immediately, but some day in the near future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Clarifying that I meant to say..."service root", not the root of the repo.

This implementation honors .ignore files in the root of the service.

I think we are on the same page.

if err != nil && !os.IsNotExist(err) {
return fmt.Errorf("reading %s file: %w", ignoreFileName, err)
}

// Temporary array to build exclude conditions dynamically
tempExcludeConditions := []excludeDirEntryCondition{}

// If there's no .ignore file, add the provided excludeConditions
if ignoreMatcher == nil {
tempExcludeConditions = append(tempExcludeConditions, options.excludeConditions...)
} else {
// If there's a .ignore file, apply ignoreMatcher only
tempExcludeConditions = append(tempExcludeConditions, func(path string, file os.FileInfo) bool {
relativePath, err := filepath.Rel(src, path)
if err == nil && dotignore.ShouldIgnore(relativePath, file.IsDir(), ignoreMatcher) {
return true
}
return false
})
}

// these exclude conditions applies to all projects
options.excludeConditions = append(options.excludeConditions, globalExcludeAzdFolder)
// Always append the global exclusions (e.g., .azure folder)
tempExcludeConditions = append(tempExcludeConditions, globalExcludeAzdFolder)

// Copy the source directory to the destination, applying the final exclude conditions
return copy.Copy(src, dst, copy.Options{
Skip: func(srcInfo os.FileInfo, src, dest string) (bool, error) {
for _, checkExclude := range options.excludeConditions {
// Apply exclude conditions (either the default or the ignoreMatcher)
for _, checkExclude := range tempExcludeConditions {
if checkExclude(src, srcInfo) {
return true, nil
}
Expand Down
13 changes: 13 additions & 0 deletions cli/azd/pkg/project/service_target.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,19 @@ type ServiceTarget interface {
) ([]string, error)
}

// GetIgnoreFileNameByKind returns the appropriate ignore file name (e.g., .funcignore, .webappignore)
// based on the service target kind.
func GetIgnoreFileNameByKind(kind ServiceTargetKind) string {
switch kind {
case AzureFunctionTarget:
return ".funcignore"
case AppServiceTarget:
return ".webappignore"
default:
return ""
}
}
Comment on lines +99 to +108
Copy link
Member Author

Choose a reason for hiding this comment

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

I debated on whether or not we should have a helper method like this, or add this to the interface, but since only a few of the service targets will have this capability I figured we should add it as a helper method to not mess with the interface of the service target. But I could be convinced the other direction, depending on how you want to design this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like flexibility -- simple functions are great for that purpose. I'm okay here.


// NewServiceDeployResult is a helper function to create a new ServiceDeployResult
func NewServiceDeployResult(
relatedResourceId string,
Expand Down
64 changes: 47 additions & 17 deletions cli/azd/pkg/rzip/rzip.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,47 +12,77 @@ import (
"strings"
)

// CreateFromDirectory compresses a directory into a zip file.
func CreateFromDirectory(source string, buf *os.File) error {
w := zip.NewWriter(buf)
defer w.Close()

// Walk through the source directory
err := filepath.WalkDir(source, func(path string, info fs.DirEntry, err error) error {
if err != nil {
return err
}

if info.IsDir() {
return nil
}
fileInfo, err := info.Info()
// Fetch file info (Lstat avoids following symlinks)
fileInfo, err := os.Lstat(path)
if err != nil {
return err
}

header := &zip.FileHeader{
Name: strings.Replace(
strings.TrimPrefix(
strings.TrimPrefix(path, source),
string(filepath.Separator)), "\\", "/", -1),
Modified: fileInfo.ModTime(),
Method: zip.Deflate,
// Skip symbolic links
if fileInfo.Mode()&os.ModeSymlink != 0 {
return nil
}

f, err := w.CreateHeader(header)
if err != nil {
return err
// Create relative path and normalize it for zip
relativePath := filepath.ToSlash(strings.TrimPrefix(strings.TrimPrefix(path, source), string(filepath.Separator)))

// Handle directories by adding a trailing slash
if fileInfo.IsDir() {
relativePath += "/"
}
in, err := os.Open(path)

// Create zip header from the file info
header, err := zip.FileInfoHeader(fileInfo)
if err != nil {
return err
}
_, err = io.Copy(f, in)

header.Name = relativePath
header.Modified = fileInfo.ModTime()

// Compress files (leave directories uncompressed)
if !fileInfo.IsDir() {
header.Method = zip.Deflate
}

// Write the header to the zip
writer, err := w.CreateHeader(header)
if err != nil {
return err
}

// Write the file's content if it's not a directory
if !fileInfo.IsDir() {
if err := writeFileToZip(writer, path); err != nil {
return err
}
}

return nil
})

return err
}

// writeFileToZip writes the contents of a file to the zip writer.
func writeFileToZip(writer io.Writer, filePath string) error {
file, err := os.Open(filePath)
if err != nil {
return err
}
defer file.Close()

return w.Close()
_, err = io.Copy(writer, file)
return err
}
Loading
Loading