-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[acr] Fix #14811 Add support for dockerignore override #14916
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
[acr] Fix #14811 Add support for dockerignore override #14916
Conversation
|
hi @fengzhou-msft could you pls help to review this? thanks |
| ignore_list, ignore_list_size = _load_dockerignore_file(source_location) | ||
| # NOTE: os.path.basename is unable to parse "\" in the file path | ||
| original_docker_file_name = os.path.basename(docker_file_path.replace("\\", "/")) | ||
| ignore_list, ignore_list_size = _load_dockerignore_file(source_location, original_docker_file_name) |
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.
where is ignore_list_size used? do you think its redundant to return that?
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.
Hi @shahzzzam, thanks for your comment. It looks like it's used in:
I am not sure if it would be appropriate to refactor that, but I personally see that as out of scope for this PR. Please let me know if you have suggestions in that sense.
|
Hi @juliusl and @shahzzzam did you have a chance to have a look at my commit and replies to your comments? |
|
LGTM, LGTY @shahzzzam ? |
shahzzzam
left a comment
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.
LGTM
|
Hi @northtyphoon, what's the next step for this? Am I waiting for another review? |
|
hi @fengzhou-msft, could you pls help to review the PR? thanks |
1 similar comment
|
hi @fengzhou-msft, could you pls help to review the PR? thanks |
Description
There is currently no way to specify (or override) the .dockerignore file in the acr build command. This prevents multiple Dockerfile(s) to share the same context effectively.
This change allows different Dockerfiles to define different subsets of the context files with different ignore rules.
In case the docker file is not named Dockerfile, the frontend (i.e. acr build) first checks for <path/to/docker_file_name>.dockerignore and if it is found it will be used instead of the root .dockerignore.
This is consistent with the docker behaviour implemented in:
moby/buildkit#901
For more information see the related issue #14811
Testing Guide
No command changes. This PR affects the
acr buildcommand, when the-f <docker_file_name>option is used.History Notes
[acr] Add support for dockerignore override
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.