Skip to content
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

setup_osie: Add --retry to curl, exit if it fails #533

Closed
wants to merge 3 commits into from

Conversation

tstromberg
Copy link
Contributor

@tstromberg tstromberg commented Sep 1, 2021

Fixes #306

Includes some minor refactoring to decrease the indentation level and adds some informational messages.

(PS: On that note, I really miss having a progress bar for curl, given that the download takes ~10 minutes here)

@tstromberg tstromberg changed the title Add --retry to osie download, make curl options more obvious setup_osie: Add --retry to curl, exit if it fails Sep 1, 2021
@tstromberg tstromberg requested a review from mmlb September 1, 2021 21:50
@codecov
Copy link

codecov bot commented Sep 1, 2021

Codecov Report

Merging #533 (c0c5aa1) into master (cbe2037) will not change coverage.
The diff coverage is n/a.

❗ Current head c0c5aa1 differs from pull request most recent head 483a925. Consider uploading reports for the commit 483a925 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #533   +/-   ##
=======================================
  Coverage   33.59%   33.59%           
=======================================
  Files          44       44           
  Lines        3387     3387           
=======================================
  Hits         1138     1138           
  Misses       2152     2152           
  Partials       97       97           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbe2037...483a925. Read the comment docs.

echo "$INFO extracting osie tar"
tar -zxf "$TB_OSIE_TAR"

if [ -d "$osie_current" ] && [ -d "$tink_workflow" ]; then
Copy link
Contributor

@mmlb mmlb Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably check for

Suggested change
if [ -d "$osie_current" ] && [ -d "$tink_workflow" ]; then
if [ -f "$osie_current/vmlinuz-x86_64" ] && [ -f "$tink_workflow/workflow-helper.sh" ]; then

instead. Otherwise if curl fails then a re-run will skip curl right? Because you've mkdir -p these dirs right after this branch

Comment on lines +284 to 294
tar -zxf "$TB_OSIE_TAR"

if pushd osie*/; then
if mv workflow-helper.sh workflow-helper-rc "$tink_workflow"; then
cp -r ./* "$osie_current"
else
echo "$ERR failed to move 'workflow-helper.sh' and 'workflow-helper-rc'"
exit 1
fi
else
echo "$INFO found existing osie files, skipping osie setup"
popd
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this should actually be:

Suggested change
tar -zxf "$TB_OSIE_TAR"
if pushd osie*/; then
if mv workflow-helper.sh workflow-helper-rc "$tink_workflow"; then
cp -r ./* "$osie_current"
else
echo "$ERR failed to move 'workflow-helper.sh' and 'workflow-helper-rc'"
exit 1
fi
else
echo "$INFO found existing osie files, skipping osie setup"
popd
fi
if ! tar -zxf "$TB_OSIE_TAR" -C "$osie_current"; then
echo "$ERR failed to extract osie tarball";
exit 1;
fi
if ! mv/cp "$osie_current/workflow-helper.sh" "$osie_current/workflow-helper-rc" "$tink_workflow"; then
echo "$ERR failed to move 'workflow-helper.sh' and 'workflow-helper-rc'";
exit 1;
fi

no need to change dir, ..., mv .* when tar can do what we want right? This way we don't have to deal with pushd errors (currently ignore, which tbf shouldn't ever really happen) but also doesn't keep 3 copies of osie around (the tarball, one in $cwd, and the one in $osie_current)... just 2 copies.

@jacobweinstock
Copy link
Member

Hey @tstromberg @mmlb. If I'm not mistaken, I don't think setup.sh is used anywhere anymore in this repo.

@jacobweinstock jacobweinstock mentioned this pull request Sep 3, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the file pull via curl is not robust and sometimes fail with out error / abort the script
3 participants