-
Notifications
You must be signed in to change notification settings - Fork 7
Stable.25 04.2 #354
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Open
uptownhr
wants to merge
2
commits into
stable.25-04
Choose a base branch
from
stable.25-04.2
base: stable.25-04
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Stable.25 04.2 #354
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 be37609 in 1 minute and 42 seconds. Click for details.
- Reviewed
60
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
7
draft 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. packages/infrastructure/wf_dockerfile_build/scripts/setup.sh:5
- Draft comment:
Quote shell variables in condition tests to avoid word splitting. - 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 quoting variables is generally good practice in shell scripts, [[ ]] tests are special and handle unquoted variables safely. The suggestion is overly pedantic here since: 1) [[ ]] prevents word splitting already 2) these are env vars unlikely to have spaces 3) even if they did have spaces, the -n/-z tests would still work correctly. Maybe there are edge cases where quoting could matter even in [[ ]] tests? Maybe this is part of a style guide requirement? Even if there are edge cases, they would be extremely rare and not impact functionality here. Style consistency isn't worth a PR comment since both forms are widely used. This comment should be deleted as it suggests an unnecessary change that provides no real benefit in this context.
2. packages/infrastructure/wf_dockerfile_build/scripts/setup.sh:11
- Draft comment:
Quote variables in URL construction to prevent unexpected behavior. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. packages/nix/packages/scripts/pf-get-commit-hash.sh:18
- Draft comment:
Minor correction: Usage text now correctly spells 'git_ref'. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
4. packages/infrastructure/wf_dockerfile_build/scripts/setup.sh:5
- Draft comment:
Ensure both GIT_USERNAME and GIT_PASSWORD are provided. Also consider URL-encoding these credentials to handle special characters and avoid accidental exposure in logs. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The first part of the comment about ensuring both credentials are provided is redundant since the code already does this check. The second part about URL-encoding credentials is a valid security concern, but it's speculative - we don't know if special characters are actually an issue in this context, and the comment doesn't provide strong evidence that this is definitely needed. The URL-encoding suggestion could be a legitimate security improvement. Maybe we're being too quick to dismiss it? While URL-encoding might be good practice, without knowing more about the actual usage patterns and requirements, it's a speculative suggestion. We should follow the rule of only keeping comments that are definitely issues. Delete the comment because the first part is already implemented, and the second part is speculative without strong evidence that it's needed.
5. packages/nix/packages/scripts/pf-get-commit-hash.sh:18
- Draft comment:
Typo fixed: changed '--ref <gi_ref>' to '--ref <git_ref>'. Documentation now correctly reflects the option name. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it only states that a typo was fixed and the documentation was updated accordingly. It does not provide any actionable feedback or suggestions for improvement.
6. packages/website/src/content/docs/changelog/stable-25-04.mdx:11
- Draft comment:
Documentation update for stable.25-04.2 looks good. Consider adding a trailing newline at the end of the file to adhere to best practices. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
7. packages/website/src/content/docs/changelog/stable-25-04.mdx:24
- Draft comment:
There is a duplicate word 'the the' in the last release note. Please remove the extra 'the' to ensure clarity. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_tvSzQT2Sra0xLHkH
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Fixes private repo authentication in
setup.sh
and updates changelog.setup.sh
, added checks forGIT_USERNAME
andGIT_PASSWORD
to ensure both are provided for private repo access.REPO
URL with credentials ifGIT_USERNAME
is provided.pf-get-commit-hash.sh
usage message (gi_ref
togit_ref
).stable-25-04.mdx
to include private repo authentication fix.This description was created by
for be37609. You can customize this summary. It will automatically update as commits are pushed.