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
jongio marked this conversation as resolved.
Show resolved Hide resolved
}

// 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.

@weikanglim - Any objections to moving forward with the current implementation as it matches what .funcignore does today and what they would like for .webappignore.

This means a service zipping only recognizes the .ignore file that is in the root of that service folder and not any parent .ignore files.

Copy link
Contributor

@weikanglim weikanglim Sep 24, 2024

Choose a reason for hiding this comment

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

@jongio Just checking -- where are we in resolving the difference between of behavior in interpretation for .funcignore by func-tools vs. azd, as highlighted previously? I haven't seen it mentioned.

Aside from that:

  1. I'm a little unhappy about dotignore being a package and the implementation of the functions being tied to working "within a directory". I suspect in the future this may end up needing slight rewriting to support recursive directories. I'm more than happy to work with you to push some changes here to clean up the implementation.
  2. I'm looking at the PR again, and it seems like there are significant changes to rzip.go that I'm unsure how or if they're related. They should be separated out if it's a bug fix.
  3. I'm also a little nervous about the e2e tests that we're doing inspection of the zip files. The heavy addition of many test data files to set up "scenario tests" of all the projects also seem rather heavyweight and will be slightly harder to maintain in the long term. It feels like simply testing the "exclusion conditions" would suffice for testing the behavior with/without *.ignore files.
  4. We should double check again what the default behavior of without .funcignore should be -- I believe it should minimally ignore local.settings.json to be inline with function tools, see docs here.

Overall, I really do love the direction here. But I would love if we can have a simple clean landing of a minimal change -- there are currently 37 file changes in this PR alone. I'm more than happy to work with you to land a minimal change if we can document all the necessary requirements in a new issue referencing #1039 so we're all on the same page, and also to serve as official documentation for the feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

On dotignore and recursive directories:

Thanks for sharing your thoughts on dotignore. The current non-recursive behavior is consistent with how .funcignore works today, and I believe it's important to avoid introducing more differences. I’d prefer to maintain this alignment to keep things predictable and straightforward for developers.

On changes to rzip.go:

The changes in rzip.go are required to handle symbolic links in node_modules or virtual environments. Since these components could now be included, special code is necessary to ensure correct behavior. Keeping these updates within this PR ensures everything works seamlessly together.

On e2e tests and test data files:

Our current e2e tests are designed to cover any issues arising from the zip changes, particularly around symbolic links. The test data may be heavier, but it provides comprehensive coverage that ensures we don’t miss any potential edge cases.

On default behavior for .funcignore:

Regarding the behavior of .funcignore, I’m in touch with the functions team to verify if there are additional files that should be ignored, with or without the existence of a .funcignore file. It's also important to note that any issues where .funcignore doesn’t behave like .gitignore should be resolved within the func implementation. I don't think we should introduce behavior here that diverges from the expected .gitignore syntax.

Thank you for the offer, but I’m confident the current implementation is in line with our goals, and I don't feel any additional simplification is needed at this time. I appreciate your willingness to assist and your thoughtful feedback throughout this process

Copy link
Contributor

@weikanglim weikanglim Sep 25, 2024

Choose a reason for hiding this comment

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

I've left a comment on rzip.go that left me with some more questions. I'd still push towards creating a separate issue for rzip.go change so that we can understand and track the changes separately.

I’m in touch with the functions team to verify if there are additional files that should be ignored

Would we consider transferring all the existing documentation on the PR into an issue so that we can have the requirements discussion? I'm also more than happy to help here if needed.

Overall, I do really want this to see these changes landed -- I'm just trying to make sure we're covering enough our bases from enough angles to make this a good user experience for our users.

Copy link
Member Author

@jongio jongio Sep 26, 2024

Choose a reason for hiding this comment

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

Here are the list of default excludes for function service type:
https://github.com/Azure/azure-functions-core-tools/blob/37b826f143b99cd62308b88320e71aa806f6197f/src/Azure.Functions.Cli/Common/FileSystemHelpers.cs#L144

Those will need to be excluded before zipping for function target.

We may also want to confirm with app service team if there are default excludes like appsettings.development.json

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 ""
}
}
jongio marked this conversation as resolved.
Show resolved Hide resolved

// 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 {
Comment on lines +32 to +33
Copy link
Contributor

@weikanglim weikanglim Sep 25, 2024

Choose a reason for hiding this comment

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

I'm curious about the motivation here to skip copying of symbolic links entirely. In my mind, the "righ" thing from an azd user perspective is that azd will dereference symbolic links and include the contents of those links in the archive.

This is also the default behavior of gnuutils: zip, stackoverflow.

It's unclear what the rest of the other changes in this file is -- I assume it's handling empty directories in the zip? I know I've received a lot of pushback here -- but would we consider creating a separate issue and PR just to have this land more smoothly?

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'm actually thinking that maybe we want to keep venvs excluded by default. I don't think we have a good use case for including venvs in a zip folder.

@pamelafox - Can you confirm this? And also do you think that we should include pycache by default if they have a .ignore file? I'm thinking it will be large and might not be needed in any scenario.

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