Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #4258base: main
Are you sure you want to change the base?
Add
.webappignore
and.funcignore
Support for Service Packaging #4258Changes from all commits
c4e6486
fbee0d5
336d4c6
3558db3
486f162
4579236
32f23f2
014ae0e
a323e42
26346cd
76c4f5f
898aeb4
da8cca6
43153f1
c4fbaeb
714fff1
26f4199
566f839
02876f7
3c25693
51cf6a7
a10080f
a08c003
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?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 adotignore
package -- I would like to see the design matured out before we push it up to an app library package inazd
.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellismg - Are you thinking we migrate the pack_source.go implementation to use the new dotignore support in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 @jongioIf we're already engaged in conversations with both app service and function teams, can we consider:
.funcignore
interpretations (is there user pain we would worry about?)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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.rzip.go
that I'm unsure how or if they're related. They should be separated out if it's a bug fix.*.ignore
files..funcignore
should be -- I believe it should minimally ignorelocal.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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.