Skip to content

When running the upload script, look for uncommitted changes and check before upload if necessary. #343

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.

Sign up for GitHub

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

Merged
Standard8 merged 1 commit into from
Jun 4, 2026
Merged

When running the upload script, look for uncommitted changes and check before upload if necessary. #343

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 48 additions & 8 deletions scripts/upload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,41 @@
from getpass import getpass
import json
import requests
import subprocess
import sys

def has_uncommitted_changes():
"""Check if there are uncommitted changes in a jj or git repository."""
# Try jj first — works for both native jj repos and git-backed jj repos.
try:
result = subprocess.run(
["jj", "diff", "--stat"],
capture_output=True,
text=True,
)
if result.returncode == 0:
# Successfully queried a jj repo; jj only tracks non-ignored files
# so no extra filtering is needed.
return bool(result.stdout.strip())
# jj is installed but this is not a jj repository; fall back to git.
except FileNotFoundError:
pass # jj is not installed

# Fall back to git, ignoring untracked files.
try:
result = subprocess.run(
["git", "status", "--porcelain", "--untracked-files=no"],
capture_output=True,
text=True,
)
if result.returncode == 0:
return bool(result.stdout.strip())
except FileNotFoundError:
pass # git is not installed

return False


API_ENDPOINTS = {
"dev": "https://remote-settings-dev.allizom.org/v1/",
"stage": "https://remote-settings.allizom.org/v1/",
Expand Down Expand Up @@ -37,6 +70,19 @@
parser.print_help()
sys.exit(1)

if has_uncommitted_changes():
if args.server == "prod":
print("ERROR: There are uncommitted local changes. Please ensure all changes have been reviewed before pushing to production", file=sys.stderr)
sys.exit(1)

# We allow pushing with uncommitted changes on dev and stage because this
# can be useful for quick tests (e.g. to help QA check something).
reply = input(
"\n\nWARNING: There are uncommitted local changes. Are you sure you wish to proceed? (y/N):"
).lower().strip()
if not reply or reply[0] != "y":
sys.exit(1)

Comment on lines +73 to +85

@mandysGit mandysGit May 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we should not allow it to proceed if there are uncommitted changes.
There could be a situation where the keyboard was accidentally and quickly typed "y", without reading the prompt.

I'd rather have the user clear all uncommitted changes first before pushing to RS. That way, we can be stricter in guaranteeing the code being pushed is from the approved Phab review without anything extra added.

@Standard8 Standard8 May 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think there's cases that we do want to potentially allow this - e.g. a quick test on dev or staging of something. Forcing commit is additional overhead, though I respect we are trying to be careful here.

How about not allowing uncommitted at all on production, but allowing it for dev & stage?

@mandysGit mandysGit Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sounds good! Let's not allow uncommitted only on production.

@mandysGit mandysGit Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

r+ with that change.

# workspace = 'main' if args.server == 'dev' else 'main-workspace'

API_ENDPOINT = API_ENDPOINTS[args.server] + "buckets/%s/collections/%s/records" % (
Expand All @@ -57,12 +103,6 @@

existingRecords = response.json()

# Handle python 2 backwards compatibility.
if sys.version_info[0] < 3:
inputFn = raw_input
else:
inputFn = input


def getIdForRecord(record):
""" Gets or creates an id based on the given record.
Expand Down Expand Up @@ -100,7 +140,7 @@ def findRecord(id, recordSet):

def yes_or_no(question):
while "the answer is invalid":
reply = str(inputFn(question + " (y/n): ")).lower().strip()
reply = str(input(question + " (y/n): ")).lower().strip()
if reply[0] == "y":
return True
if reply[0] == "n":
Expand Down Expand Up @@ -157,7 +197,7 @@ def strip_record(record):
recordsToRemove.append(record)

if len(recordsToRemove) > 0:
print("\Records to Remove:\n")
print("\nRecords to Remove:\n")

for record in recordsToRemove:
print(getIdForRecord(record))
Expand Down