-
Notifications
You must be signed in to change notification settings - Fork 495
Update ci.yaml #711
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
base: master
Are you sure you want to change the base?
Update ci.yaml #711
Conversation
|
@ma96-gif is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
ma96-gif
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.
Review
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.
Important
Looks good to me! 👍
Reviewed everything up to 8ac6f72 in 1 minute and 23 seconds. Click for details.
- Reviewed
45lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/ci.yaml:16
- Draft comment:
The job name and comment now reference '.nmv' instead of '.nvmrc'. Confirm that this renaming is intentional, as '.nvmrc' is the standard file for Node version specification. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While '.nvmrc' is indeed the standard file name, this appears to be an intentional rename throughout the file. The comment is essentially asking for confirmation of intention, which violates our rules. We don't have context about why they chose to rename it, and we should assume they had a good reason. The build would fail if this was incorrect. The comment does raise a valid point about deviating from standard conventions, which could affect maintainability and developer expectations. However, asking for confirmation of intention is explicitly against our review rules, and we should assume the author had a valid reason for the rename. Delete the comment as it primarily asks for confirmation of intention and doesn't point out a clear issue that requires fixing.
2. .github/workflows/ci.yaml:26
- Draft comment:
Sparse-checkout now references '.nmv'. Please verify that the repository contains this file and that all tooling expects '.nmv' instead of '.nvmrc'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to verify the presence of a file and the expectations of tooling, which is against the rules. It does not provide a specific suggestion or ask for a test to be written. It is more of a request for confirmation, which is not allowed.
3. .github/workflows/ci.yaml:31
- Draft comment:
The step reading the Node version now uses '.nmv'. Ensure this change is deliberate and consistent with the overall project configuration. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that the change is deliberate and consistent, which violates the rule against asking the author to confirm their intention or ensure consistency. It does not provide a specific suggestion or ask for a specific test to be written.
4. .github/workflows/ci.yaml:1
- Draft comment:
The PR description mentions resolving a transaction history issue, but the changes here only update CI configuration. Please ensure the PR title, description, and commits are aligned with the intended fix. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. .github/workflows/ci.yaml:16
- Draft comment:
Typographical observation: The job comment now reads 'Job to read Node version from .nmv'. Please confirm if '.nmv' is the intended filename, as it appears it might be intended to be '.nvmrc'. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. .github/workflows/ci.yaml:29
- Draft comment:
Typographical observation: The step name has been updated to 'Read .nmv'. Verify that this is correct and not a typo for '.nvmrc'. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_TmzNyskAozAvcOaz
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
That PR shall break the last mile |
|
@jacobcreech, @Woody4618 could you close this? |
|
@jacobcreech bump |
Description
Type of change
Screenshots
Testing
Related Issues
Checklist
Additional Notes
Important
Rename
.nvmrcto.nmvand update related job and step identifiers inci.yaml..nvmrcto.nmvinci.yaml.read-nvmrctoread-nmv.nvmrctonmv.node-versionoutput inBuild-And-Testjob to useread-nmvoutputs.This description was created by
for 8ac6f72. You can customize this summary. It will automatically update as commits are pushed.