Skip to content

Conversation

@gspencergoog
Copy link
Collaborator

Description

Removing redundant command after gcloud CLI migration in #610

The automated migration revealed a duplicate command line, and presumably the gcloud CLI has fixed the parallelism problems.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request simplifies the script by removing a redundant gcloud storage cp command, which was likely a workaround for a previous issue. While this is a good simplification, the current implementation will silently ignore failures during the file upload and incorrectly report success. I've suggested a change to properly handle the command's success or failure, providing clearer feedback to the user and making the script more robust.

Comment on lines +138 to 139
gcloud storage cp learner_context/*.txt "gs://${CONTEXT_BUCKET}/learner_context/" 2>/dev/null|| true
echo " Learner context uploaded to gs://${CONTEXT_BUCKET}/learner_context/"
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The current implementation suppresses errors and continues execution, which means a failure to upload files will go unnoticed while the script reports success. This can be misleading and make debugging difficult. It's better to check the exit code of the command and provide appropriate feedback to the user.

Suggested change
gcloud storage cp learner_context/*.txt "gs://${CONTEXT_BUCKET}/learner_context/" 2>/dev/null|| true
echo " Learner context uploaded to gs://${CONTEXT_BUCKET}/learner_context/"
if gcloud storage cp learner_context/*.txt "gs://${CONTEXT_BUCKET}/learner_context/" >/dev/null 2>&1; then
echo " Learner context uploaded to gs://${CONTEXT_BUCKET}/learner_context/"
else
echo -e "${YELLOW}Warning: Failed to upload learner context files. The demo may not be fully functional without them.${NC}"
fi

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, ideally this wouldn't ignore errors, but the original script did. Should we just take off the || true?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah maybe. I am not sure where this sample came from

@gspencergoog gspencergoog merged commit 1c20b8c into google:main Feb 10, 2026
5 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in A2UI Feb 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants