[DOP-5031]: Add persistence module functionality to script #9
Conversation
✅ Deploy Preview for search-manifest-integration canceled.
|
✅ Deploy Preview for persistence-module-integration canceled.
|
✅ Deploy Preview for site-links-display canceled.
|
✅ Deploy Preview for redoc-integration canceled.
|
✅ Deploy Preview for snooty-cache-plugin ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -0,0 +1,29 @@ | |||
| import type { NetlifyPluginUtils } from '@netlify/build'; | |||
There was a problem hiding this comment.
Actual change here
snooty-cache/src/index.ts
Outdated
|
|
||
| await checkForNewSnootyVersion(run); | ||
| }, | ||
| await downloadPersistenceModule(run); |
There was a problem hiding this comment.
actual change here
✅ Deploy Preview for git-changed-files-plugin-bianca ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
branberry
changed the title
There was a problem hiding this comment.
Just a couple of questions from me right now!
Also might be a nitpick but the PR title suggests we're calling the persistence module here, but PR description suggests that will be handled separately? Should we update the title to more accurately reflect the change?
I also see some linting errors in the CI, despite all of the linting changes in the PR 🤔 Might be out of scope, but would we ever want to make the linting CI with formatting a required check to ensure PRs don't have mass changes in the future?
| if (isModuleDownloaded) return; | ||
|
|
||
| await run.command( | ||
| 'git clone --depth 1 --filter=tree:0 https://github.com/mongodb/docs-worker-pool.git --sparse', |
There was a problem hiding this comment.
(Questions)
- Do we have plans to port over the persistence module directly to this repo in the future?
- Is there a reason why we're downloading the persistence module from the Autobuilder repo while also having
persistence-modulecode in this repo?
There was a problem hiding this comment.
Great questions!
For your first point, that was the original intention, but the one issue with porting it over as an integration is that we need the persistence module to run prior to building the Gatsby site.
I thought it would be easier to do so, but the order in which the integrations run make it difficult to do that since the build.sh script calls the parser and then calls the Gatsby site. The build integrations can essentially only be run before or after this point.
To answer your question, I think right now we don't have plans to port over all of the persistence module code at this time. This doesn't mean we shouldn't, though. One thing that caused this change of plans is that I'm unsure of how much will need to change with the logic in the persistence module with regards to some upcoming projects (I'll message you on Slack more on this).
For you second point, thank you for calling that out. There isn't a reason for that, and I'll make sure to remove the extra code 👍
| await downloadPersistenceModule(run); | ||
| } catch (e) { | ||
| console.error('Unable to run the persistence module', e); |
There was a problem hiding this comment.
(Nit) Is this function for downloading or for running the persistence module? Differences in the name of the function and error message are a bit confusing to me.
(Question) The context might be lost on me, but can you also please briefly remind me why the caching integration is responsible for calling the persistence module in this case? I thought this integration was responsible for caching parser data. Hoping to just learn more about this!
There was a problem hiding this comment.
Hey Raymund! Really good point, and an oversight on my part. This should really just live in the persistence module integration instead of this one.
I'll move this code 👍
branberry
changed the title
|
@rayangler Thank you for the review! To your question of the formatting changes, I think it would be a good idea to do that. I actually ran the linter and formatter manually this time because this hasn't been run on the codebase yet. There are some other linting changes that couldn't manually be resolved automatically such as typing. Either way, these are formatting changes that are required, so I'll probably just run the same command and create a separate PR to make it more clear what's going on. |
Notes
Add the persistence module as a part of the Netlify builds. Another PR will be introduce shortly to add the command that calls the persistence module.
Apologies, ran the formatter + linter which produced a TON of changes. I've marked where the actual changes live as PR comments
Testing
Successfully ran locally with
docs-node