-
Notifications
You must be signed in to change notification settings - Fork 7
fix: Ensure utils:curl
re-runs when input URLs or the library target's commit hash changes.
#24
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: main
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CT as Curl Task
participant S as Shell
U ->> CT: Invoke curl task
CT ->> S: Download file (OUTPUT_FILE)
S -->> CT: File downloaded
CT ->> S: Compute & compare SHA256 hash for OUTPUT_FILE
S -->> CT: Hash verification result
CT ->> S: Echo URL into URL_FILE
CT ->> S: Compare expected URL with URL_FILE content
S -->> CT: URL verification result
CT ->> U: Return verification result
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
taskfiles/utils-remote.yaml (2)
17-17
: Consider using a more robust path construction method.The current implementation replaces all periods with hash symbols, which could lead to unexpected results if
OUTPUT_FILE
contains multiple periods.Consider using a more targeted approach:
- URL_FILE: "{{.OUTPUT_FILE | replace \".\" \"#\"}}.url" + URL_FILE: "{{dir .OUTPUT_FILE}}/{{base .OUTPUT_FILE | trimSuffix (ext .OUTPUT_FILE)}}#url"
34-34
: Consider moving URL file creation after successful download.Creating the URL file before attempting the download could lead to an inconsistent state if the download fails.
Consider moving the URL file creation inside the success branch:
- echo "{{.URL}}" > "{{.URL_FILE}}" max_attempts=3 attempt=1 while [ $attempt -le $max_attempts ]; do if curl \ --fail \ --location \ --show-error \ --connect-timeout 10 \ --max-time 300 \ "{{.URL}}" \ --output "{{.OUTPUT_FILE}}"; then + echo "{{.URL}}" > "{{.URL_FILE}}" break fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
taskfiles/utils-remote.yaml
(3 hunks)
🔇 Additional comments (1)
taskfiles/utils-remote.yaml (1)
22-26
: LGTM! Well-structured URL verification.The URL verification is implemented consistently with the existing hash verification pattern.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
taskfiles/utils-remote.yaml (1)
56-61
: Standardize hash terminology.While the additional hash verification is valuable for user feedback, let's maintain consistent terminology throughout the codebase.
Apply this diff to align with the documentation:
- - "echo Verifying the commit hash of {{.OUTPUT_FILE}}." + - "echo Verifying the SHA256 hash of {{.OUTPUT_FILE}}."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
taskfiles/utils-remote.yaml
(3 hunks)
🔇 Additional comments (3)
taskfiles/utils-remote.yaml (3)
17-17
: LGTM! Well-structured variable definition and generates section.The variable naming and file path construction are clear and follow best practices. The generates section correctly declares both output files.
Also applies to: 20-20
22-25
: LGTM! Effective URL verification in status check.The status check will properly trigger a re-run when the URL changes, addressing the core issue mentioned in the PR objectives.
34-34
: LGTM! URL tracking implementation.The URL file creation is properly placed and will maintain a record of the source URL.
utils:curl
to run when input urls or commit hash for the library target to be downloaded changes.utils:curl
re-runs when input URLs or the library target's commit hash changes.
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.
Any reason we can't just add the SHA to the task label
rather than storing a separate file?
Also, is it possible to just add some output in status
?
I think the |
Description
Task
can't force re-runs by detecting input argument changes. Without this PR,utils:curl
will stay ' up-to-date' if we change only theurl
of the tar source but not itsFILE_SHA256
.Also in such situations where the tar source downloaded doesn't match the given
FILE_SHA256
, no error message is outputted. The task simply re-runs over and over again without users knowing that they have supplied a wrongFILE_SHA256
.To solve the problems above:
URL
variable as plain text. On subsequent runs, we force the task to be re-run if the suppliedURL
becomes different and fails the task'sstatus
check.status
simply runs the curl operation repeatedly.Checklist
breaking change.
Validation performed
ystdlib-cpp
by stimulating the adjusting of input arguments toutils:curl
.utils:download-and-extract-tar
andutils:cmake-install-remote-tar
.