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
Fix: Cache file error handling on read-only file system. #11946
Conversation
@ilyavolodin @mysticatea Are you confirming this is a bug (you've both commented here but we haven't accepted the issue yet)? Just want to make sure we're all on the same page. |
No, I haven't been able to verify it, but I also don't have an environment to verify it in. I'm going purely on code changes. |
Same (I have not reproduced it). But because the current code re-throws IO errors except |
@mysticatea @ilyavolodin I've changed to code to make an exception for |
@cuki Thanks, that looks better to me. But I still don't know how to verify this to mark it accepted. I don't have an environment to do so. |
@ilyavolodin Here are the steps to reproduce the 1. Create a "project" directoryWhich consists out of the following files: DockerfileFROM node:8-stretch
RUN mkdir /src
RUN npm install -g eslint@6.0.1
WORKDIR /src
ENTRYPOINT ["/usr/local/bin/eslint"] docker-compose.ymlversion: '3.4'
services:
eslint:
build: .
read_only: true
volumes:
- .:/src:ro,cached .eslintrc.jsmodule.exports = {
"extends": "eslint:recommended"
}; test.jsfunction hello(){
console.log('world')
}
hello(); 2. Build Docker imageExecute following command inside your "project" directory: docker-compose build 3. Run ESLint in DockerStay in your "project" directory and run:
Now you should see something like the following as output:
4. Test proposed changesChange Dockerfile in your "project" directory to be as followed: FROM node:8-stretch
RUN mkdir /src
RUN npm install -g eslint@6.0.1
# Override relevant file with proposed changes
RUN git clone --single-branch -b patch-1 https://github.com/cuki/eslint /tmp/eslint/ && \
cp /tmp/eslint/lib/cli-engine/cli-engine.js /usr/local/lib/node_modules/eslint/lib/cli-engine/cli-engine.js
WORKDIR /src
ENTRYPOINT ["/usr/local/bin/eslint"] Rebuild Docker image (with proposed changes)
Run ESLint again in Docker:
And notice the output, which threw out an error before, now should be:
|
@ilyavolodin @mysticatea Did either one of you managed to reproduce the issue (and test proposed changes) using the steps from the above (previous) comment? Would be greatly appreciated to get some feedback to see whether this is an issue with my environment or in general. Thanks! |
Sorry, I was not able to test this, I do not have docker installed, and I currently do not have time to go through installation and configuration of a new environment. Maybe somebody else on the team can do it? |
OK, I confirmed it. Thank you! |
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, thank you.
@mysticatea @ilyavolodin Thanks for the quick response and confirming the issue/fix! |
What is the purpose of this pull request?
[x] Bug fix (template)
Fixes #11945.
What changes did you make?
Add additional exception for
EROFS
(when cache file does not exist) next to the already existingENOENT
exception upon trying to delete cache file.