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

Refactor/gdrive spawn #2091

Merged
merged 4 commits into from
Oct 6, 2023
Merged

Refactor/gdrive spawn #2091

merged 4 commits into from
Oct 6, 2023

Conversation

chrismclarke
Copy link
Member

@chrismclarke chrismclarke commented Oct 3, 2023

PR Checklist

  • PR title descriptive (can be used in release notes)

Description

As part of trying to unblock/retain some of the in-progress work being done in #2061

Currently all gdrive workspace scripts are executed using spawned commands from the terminal, e.g.

yarn workspace @idemsInternational/gdrive-tools start watch --folder-id 1234abc

This PR refactors to separate the command line interface (CLI) from main methods (lib), and exposes the lib methods so that they can be imported and called from code.

This should in theory make them slightly quick to execute (spawning child processes takes some time, although there are some memory management trade-offs depending on local system), but more importantly can be bundled into a build (future desktop app requirement).

Review Notes

Should simply need to test the yarn workflow sync command still downloads from google drive as expected. Would be useful to confirm the --content-watch configuration also works as well as multiple deployment sources

Dev Notes

The majority of code changes are simply splitting cli+lib code from one file to two. The more considered changes are those to the providers, and how spawnSync commands have been replaced by import

Git Issues

Closes #

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

@chrismclarke chrismclarke temporarily deployed to debug October 3, 2023 17:21 — with GitHub Actions Inactive
@chrismclarke chrismclarke requested review from jfmcquade and esmeetewinkel and removed request for esmeetewinkel October 3, 2023 17:30
@chrismclarke chrismclarke temporarily deployed to debug October 3, 2023 18:18 — with GitHub Actions Inactive
Copy link
Collaborator

@jfmcquade jfmcquade left a comment

Choose a reason for hiding this comment

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

Looks good, nice to have the methods themselves exposed.

I've tested running yarn workflow sync (including with the --content-watch flag) for various deployments. All seems to work as expected, except in the case of running yarn workflow sync --content-watch with a deployment that has multiple sheets folders, i.e. debug_gdrive_multiple. Doing so returns the following error:
Screenshot 2023-10-05 at 13 46 05

This is due to the sync_watch workflow using the deprecated sheets_folder_id value (see line 177 of packages/workflows/src/sync.workflows.ts). This is a knock-on from #2088 rather than an issue caused by this PR, so it could be dealt with in a follow-up or included here.

@chrismclarke chrismclarke temporarily deployed to debug October 6, 2023 15:55 — with GitHub Actions Inactive
@chrismclarke
Copy link
Member Author

Thanks for checking @jfmcquade , the content-watch issue makes sense and should hopefully be an easy fix. I'll make a follow-up PR so that we can get this one in first.

@chrismclarke chrismclarke merged commit 01591f1 into master Oct 6, 2023
7 checks passed
@chrismclarke chrismclarke deleted the refactor/gdrive-spawn branch October 6, 2023 16:22
@chrismclarke chrismclarke mentioned this pull request Oct 6, 2023
1 task
@chrismclarke chrismclarke mentioned this pull request Feb 1, 2024
1 task
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.

2 participants