-
Notifications
You must be signed in to change notification settings - Fork 1
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
Respect env.ALL_CHANGED_FILES if present #14
Conversation
If it's absent, the fall back to the method that might give false positives. Issue: https://khanacademy.atlassian.net/browse/FEI-3796
// uses: jaredly/get-changed-files@absolute | ||
// id: changed | ||
// with: | ||
// format: 'json' | ||
// absolute: true |
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 assume this is coming in another diff - but why the change to absolute file paths and outputting as 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.
So:
- outputting as json is more robust
- the get-changed-files action produces files relative to the github base by default; I added the absolute option because that's what the rest of our system expects, and it allows us to run commands from various subdirectories, not just the workspace root.
git-changed-files.js
Outdated
const files = JSON.parse(process.env.ALL_CHANGED_FILES) | ||
return files.filter(path => !isFileIgnored(cwd, path)) |
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.
You may have to manually run the build/lint process (I see that Prettier hasn't run 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.
@jeresig so we don't have it configured on this repo 🙃
I'll add a prettierrc
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.
Oh whoops! haha
## Summary: See Khan/actions-utils#14 ## Test plan: I updated the workflow to set up the ALL_CHANGED_FILES variable. Our other workflows that use eslint-action will want to add those setup steps as well. Author: jaredly Reviewers: jeresig, jaredly Required Reviewers: Approved by: jeresig Checks: ❌ lint_and_unit, ✅ autofix Pull request URL: #28
Summary:
If it's absent, the fall back to the method that might give false positives.
Issue: https://khanacademy.atlassian.net/browse/FEI-3796
Test plan:
This pull-request shows the new code in action https://github.com/Khan/eslint-action/pull/27/files