[NO QA] Fix 62656 lint changed#62993
Conversation
|
|
|
@rafecolton see my comment #62656 (comment). If you don't want to proceed that way then this issue is [Edit: issue on remote version of the fix merge-base does not return anything] |
|
Ok I have made the proper changes so that the lint error shows up now. I needed to add a |
|
Currently working on creating the API call for merge-base. |
|
@rafecolton I made the necessary changes but I am getting an issue with the validate github actions workflow. I believe there is an error? Apply the following patch: diff --git .github/scripts/validateActionsAndWorkflows.sh .github/scripts/validateActionsAndWorkflows.sh
index fadb39c88e4..ae4e6197a05 100755
--- .github/scripts/validateActionsAndWorkflows.sh
+++ .github/scripts/validateActionsAndWorkflows.sh
@@ -86,7 +86,7 @@ else
fi
info 'Linting workflows...'
-./actionlint -color || EXIT_CODE=1
+./actionlint -color -verbose || EXIT_CODE=1
if [[ "$EXIT_CODE" == 0 ]]; then
success 'Workflows passed actionlint!'
fiThe verbose output shows no errors output on the actionlint, but it still returns 1: It looks like there was another similar failed job here: |
|
Otherwise I have implemented new logic for checking out the main repo with a calculated depth in 2389b62c0d9c5afcc938fc826542693fdd827a92. Let me know what you think. |
|
It looks like you are still pushing changes, so I'm going to unsubscribe. Please @ me when you are ready for me to review again and I will review all at once 👍 |
MacroLens
force-pushed
the
fix_62656_lint_changed
branch
from
2686b95 to
78b68a0
Compare
|
I accidentally merged and messed up the files changed, so I cherrypicked the commits around the merge and merge revert. |
|
@rafecolton Due to me messing with the history I can no longer reproduce the test situation where we had multiple lint errors when checking out c1b62b3a72e57806ccd9eb727fc139de09a7b0be. Changes still work and can be reviewed but to test this we may want to rebase changes on a commit that has errors again? Let me know if you have any good ideas. I won't be making further changes to the code, until after you review. |
|
You should be able to rebase and force push, or create a new branch, cherry pick your changes, and force push. If you are not comfortable force pushing or at all uncertain, I recommend creating a new branch instead. You are welcome to close this PR and open a new one if needed, you can @ me and I will assign myself as a reviewer |
MacroLens
force-pushed
the
fix_62656_lint_changed
branch
2 times, most recently
from
d8963ac to
862c7e7
Compare
For testing purposes this commit introduces a useOnyx lint error by removing a canBeMissing error.
Add a script for error handling of git merge-base and git diff for checking for changes between the common ancestor of origin/main and the current branch HEAD. Updates npm run to use this script.
Change the depth of the fetch so that we pull enough commits to run git merge-base and see the common ancestor.
Remove the fetch origin main from the ESLint deprecation warning. Add the fetch-depth param to checkout.
Fix style and add additional bash safety to variable assignments. Add new rerun in NPM context checks. Combine error cases for merge-base.
Unify style on curly braces and semicolons. Remove the no-pager option from the diff output
Move the NPM checking logic to top of script. Add CI variable checks to detect if local or on CI. Clean some comments. This commit is a stopgap using depth=30000 until a better solution is implemented.
Add logic to the lint-changed workflow to query for a merge-base, then calculate the number of commits from the base to the HEAD, and perform a checkout with minimal depth needed. Updated lintChanged.sh script to remove CI specific code, and use the same logic.
This reverts commit 8e8d27075d3e5d00b42d200d264d5980747a5758. Bug was introduced
When count is zero, we still want to fetch. Also we want to do shallow fetch, zero fetch-depth is all history
Use ternary logic to replace the max operation for github actions
MacroLens
force-pushed
the
fix_62656_lint_changed
branch
from
606c95f to
f3be8ab
Compare
This reverts commit 6f37361. Unnecessary cspell changes.
| if: ${{ github.actor != 'OSBotify' || github.event_name == 'workflow_call' }} | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Fetch merge base SHA from API |
There was a problem hiding this comment.
clever! It's annoying that git doesn't have this kind of thing built-in to a command like ls-remote or whatever
There was a problem hiding this comment.
This action doesn't actually do anything 🙈 it just returns the same value as ${{ github.event.pull_request.base.sha }}
There was a problem hiding this comment.
🙈 This is what I get for reading two year old github community posts.
There was a problem hiding this comment.
I'll test and simplify as needed. @rafecolton thanks for pointing that out.
| - name: Fetch merge base SHA from API | ||
| id: merge_base | ||
| run: | | ||
| MERGE_BASE_SHA="$(gh api "repos/$REPO/compare/${PR_BASE_SHA}...${PR_HEAD_SHA}" | jq -r '.merge_base_commit.sha')" |
There was a problem hiding this comment.
NAB because I don't feel strongly, but elsewhere we use the --jq flag built-in to the gh cli, and I slightly prefer that over | jq .... It just feels cleaner to me to do it in one command rather than a pipeline 🤷🏼
There was a problem hiding this comment.
Also NAB, but I don't think the local variable is affording us any additional readability here. I'd do it all in one command:
run: echo "MERGE_BASE_SHA=$(gh api "repos/$REPO/compare/${PR_BASE_SHA}...${PR_HEAD_SHA}" --jq '.merge_base_commit.sha')" >> "$GITHUB_OUTPUT"
There was a problem hiding this comment.
NAB because I don't feel strongly, but elsewhere we use the
--jqflag built-in to theghcli, and I slightly prefer that over| jq .... It just feels cleaner to me to do it in one command rather than a pipeline 🤷🏼
You and @rafecolton should have a debate lol. I actually put in a different suggestion from Rafe to use the pipe here instead of option.
To the local variable, sure.
| if: ${{ github.actor != 'OSBotify' || github.event_name == 'workflow_call' }} | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Fetch merge base SHA from API |
There was a problem hiding this comment.
This action doesn't actually do anything 🙈 it just returns the same value as ${{ github.event.pull_request.base.sha }}
|
I will address two things: Why +2 for +2 For Fetching.Suppose your history looks like this where you haven't merged where So now when we fetch with depth In the case of your history looking like you just merged main to your dev branch: Here your merge-base has moved to Why fetch in the script?From @roryabraham
This is unnecessary, because we already did the fetch with a fetch-depth in the action, this fetch is just really only associating origin/main with the history we checked out. As an example you can run the following commands in your terminal: We checked out linux-kernel with depth=1, running "So why have the command if it does nothing?": We have the fetch command here because we need to accommodate the workflow of local development AND in the case of the PR Checkout workflow because git doesn't recognize the object name Making some modifications to see on the set +e
set -x
# Fetch the commit history to include the merge-base commit
info "Fetching origin/main"
git rev-list --count origin/main
git fetch origin main --no-tags
git rev-list --count origin/mainOn the workflow when we run But after we fetch it we just get: BonusFrom @rafecolton
From @roryabraham
If we look at the It states the number of commits in the history. If you rebase or merge, the count of commits in the history could change unexpectedly, but the count we get from the API and we use in the If you ran that linux test again but with depth=10: You would actually see a rev-list count of 79. So the history itself is longer than the number of commits we fetched. This is OK, the depth we get in the A more accurate test would be to look at the the commits including merge commits in Linux. we get ConclusionI hope this addresses your questions. @roryabraham and @rafecolton please give me more feedback on this if you don't believe me or the tests I ran on my fork. If you want more detailed testing and you can come up with a procedure I can do that on the fork and link here, just send me what you think is off. Edit: In the bonus section I mistakenly used |
Use github.token instead of secrets. Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
Remove unnecessary call to the GH api for getting merge-base.
|
Heads up that the latest failure on the validate github actions has to do with an outage at GitHub in relation to workflows and actions and needs to be rerun. See the status page. |
|
Schema validation issue has been fixed in #63608, please pull main |
|
Just waiting on the inline comment now to explain the +2 😊 |
Nice work! |
|
@rafecolton just pushed the inline comment, is this enough? |
|
Not really, somebody who looks at it after the fact is not going to know what it means. I was hoping to see something more like what you wrote in the |
|
Oh. I didn't realize how much detail you wanted. What would say is the maximum number of lines it should be haha.
…-------- Original Message --------
On 6/5/25 5:45 PM, Rafe Colton wrote:
rafecolton left a comment [(Expensify/App#62993)](#62993 (comment))
Not really, somebody who looks at it after the fact is not going to know what it means. I was hoping to see something more like what you wrote in the +2 For Fetching section in [#62993 (comment)](#62993 (comment)) - ascii diagrams and all 😄
—
Reply to this email directly, [view it on GitHub](#62993 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ANANN7UMPSWLQM3HDJILXZD3CC26JAVCNFSM6AAAAAB6DZTKHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSNBWGQYTQOJSHA).
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
I don't have a maximum in mind, we don't pay by the byte 😉 It should be however long it needs to be so somebody with no prior context can understand why it's there without needing to read through the comments on this PR. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
recheck |
I will gladly draw you some ASCII art for per byte pay 💵. |
|
Thank you for all your hard work here! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/rafecolton in version: 9.1.63-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.1.63-6 🚀
|
4 participants
Explanation of Change
Changes the
lint-changedworkflow and Node script to usegit merge-baseto determinewhere to do
git diffwhen computing changed files for use in linter.Fixed Issues
$ #62656
PROPOSAL: #62656 (comment)
Tests
nvm install && npm install.npm run lint-changed, note that there is more than 1 error.npm run lint-changed, note that there is now only 1 error.This test will show on step 5 show no errors after 18af2cf is reverted.
Offline tests
N/A no changes in app.
QA Steps
N/A GitHub actions and development scripts.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop