Skip to content

fixes deploy mac and tag release #11017

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fixes deploy mac and tag release #11017

wants to merge 1 commit into from

Conversation

mekarpeles
Copy link
Member

@mekarpeles mekarpeles commented Jul 13, 2025

Closes #11019

Fixes deploys

Technical

Testing

Screenshot

Stakeholders

@Copilot Copilot AI review requested due to automatic review settings July 13, 2025 22:55
@mekarpeles mekarpeles added Affects: Admin/Maintenance Issues relating to support scripts, bots, cron jobs and admin web pages. [managed] Priority: 1 Do this week, receiving emails, time sensitive, . [managed] labels Jul 13, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes several issues in the deployment script and adds new tag/release workflows.

  • Defines OL_REPO globally and updates all tag_deploy calls to use it
  • Changes how the latest tag is determined and adds tag and release subcommands
  • Adjusts interactive prompts in deploy_wizard for more intuitive defaults
Comments suppressed due to low confidence (3)

scripts/deployment/deploy.sh:611

  • The new tag and release subcommands should be documented in the script’s usage/help section so users know they are available.
elif [ "$1" == "tag" ]; then

scripts/deployment/deploy.sh:568

  • The new conditional flow in deploy_wizard (skipping deploy and optionally checking sync) isn't covered by automated tests. Consider refactoring for testability or adding tests for all interactive paths.
    read -p "[Now] Run openlibrary deploy & audit now? [Y/n]..." answer

scripts/deployment/deploy.sh:406

  • [nitpick] Removing the PID from the background-start message makes it harder to trace processes during debugging. Consider restoring the PID output for better visibility.
        echo "(started in background)"

Comment on lines +623 to +624
echo "The Open Library weekly deploy is now complete. See changes here: https://github.com/internetarchive/openlibrary/releases/tag/$LATEST_TAG_\
NAME. Please let us know @here if anything seems broken or delightful!"
Copy link
Preview

Copilot AI Jul 13, 2025

Choose a reason for hiding this comment

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

The backslash-newline in the echo command splits the variable name and can break the URL. Consolidate the URL and variable reference on a single line or correctly reference $LATEST_TAG_NAME without a line continuation.

Suggested change
echo "The Open Library weekly deploy is now complete. See changes here: https://github.com/internetarchive/openlibrary/releases/tag/$LATEST_TAG_\
NAME. Please let us know @here if anything seems broken or delightful!"
echo "The Open Library weekly deploy is now complete. See changes here: https://github.com/internetarchive/openlibrary/releases/tag/$LATEST_TAG_NAME. Please let us know @here if anything seems broken or delightful!"

Copilot uses AI. Check for mistakes.

Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

One bug, and one spot where some duplicated code's been added. Merge at your discretion, I can't decide how problematic that bug is -- we might want to add at least a warning or a notice. I'm a bit tight on time today so can't fix these rn. We can unblock deploys by having Jim make the PID fix locally, that should fix the deploy if his version of bash also for some reason errors with the -1 index.

@@ -23,6 +23,7 @@ set -e

# See https://github.com/internetarchive/openlibrary/wiki/Deployment-Scratchpad
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
OL_REPO="$(cd "$SCRIPT_DIR/../../" && pwd)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This introduces a bug; the HEAD will be from the directory running the script, not from the version of open library being deployed. So the commit we'll be labelling as the deploy won't necessarily be what was deployed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the problem with using temporary directories and cleanup is any time the deploy breaks part of the way through, we don't have any chance to recover and the tag that we made is more or less lost anyway.

Comment on lines +617 to +624
elif [ "$1" == "release" ]; then
LATEST_TAG_NAME=$(git -C "$OL_REPO" describe --tags --abbrev=0)
echo "[Now] Generate release: https://github.com/internetarchive/openlibrary/releases/new?tag=$LATEST_TAG_NAME"
read -p "Press Enter to continue..."

echo "[Now] Deploy complete, announce in #openlibrary-g, #openlibrary, and #open-librarians-g:"
echo "The Open Library weekly deploy is now complete. See changes here: https://github.com/internetarchive/openlibrary/releases/tag/$LATEST_TAG_\
NAME. Please let us know @here if anything seems broken or delightful!"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is duplicating the code in wizard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Affects: Admin/Maintenance Issues relating to support scripts, bots, cron jobs and admin web pages. [managed] Priority: 1 Do this week, receiving emails, time sensitive, . [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploy Script fails at tagging releases
2 participants