-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(GGs): implement update operation #889
Conversation
logger.warn(`Rolling repo ${repoName} back to ${commitSha}`) | ||
return ResultAsync.fromPromise( | ||
this.git | ||
.cwd(`${EFS_VOL_PATH}/${repoName}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: better to use path.join
instead of template literals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think same comment with Harish, mainly to keep the code consistent. I think we never really used path.join
before, might be a discussion we should have team-wide.
filePath: string | ||
): ResultAsync<fs.Stats, NotFoundError | GitFileSystemError> { | ||
return ResultAsync.fromPromise( | ||
fs.promises.stat(`${EFS_VOL_PATH}/${repoName}/${filePath}`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: better to use path.join
instead of template literals?
.andThen(() => | ||
ResultAsync.fromPromise( | ||
fs.promises.writeFile( | ||
`${EFS_VOL_PATH}/${repoName}/${filePath}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use path.join
): Promise<GitCommitResult> { | ||
if (this.isRepoWhitelisted(sessionData.siteName)) { | ||
logger.info("Updating file in local Git file system") | ||
const filePath = directoryName ? `${directoryName}/${fileName}` : fileName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: path.join
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with some non-blocking nit comments
a85859a
to
8e4a180
Compare
Problem
Closes IS-400.
Solution
Breaking Changes
Features:
Tests
Deploy Notes
None