-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Fix broken new media uploads in Git Gateway. #1221
Conversation
Deploy preview for cms-demo ready! Built with commit e806679 |
Deploy preview for netlify-cms-www ready! Built with commit e806679 |
const isPrivateRepo = await this.api.isPrivateRepo(); | ||
if (isPrivateRepo) { | ||
if (!this.isPrivateRepo) { | ||
this.isPrivateRepo = await this.api.checkPrivateRepo(`${this.api_root}/repos/${repo}`); |
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.
The value is cached here since GitHub limits unauthenticated API requests. The only downside is that you need to reload the CMS page if you change the repo from public to private.
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.
I think that's a fine compromise.
src/backends/git-gateway/API.js
Outdated
checkPrivateRepo(url) { | ||
const cacheBuster = new Date().getTime(); | ||
return fetch(`${url}?ts=${cacheBuster}`) | ||
.then(response => response.ok); |
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.
It might be a good idea to check if we get a 403 (rate limited) error here, and assume that the repo is private in that case so that thumbnails don't break.
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.
That makes sense.
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.
Besides my one change request, this solution works if we're okay with using unauthenticated requests. I'd really rather not, but I haven't looked into what other options we have. I'll do so now.
@@ -139,8 +139,10 @@ export default class GitHub { | |||
let url = `https://raw.githubusercontent.com/${repo}/${this.branch}${path}`; | |||
|
|||
// Assets uploaded to private repos will need valid access tokens. | |||
const isPrivateRepo = await this.api.isPrivateRepo(); | |||
if (isPrivateRepo) { | |||
if (!this.isPrivateRepo) { |
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.
This condition would result in an unauthenticated check on every persist for public repos, maybe we should check for the presence of a Boolean instead.
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.
Oops, thank you!
@erquhart Great, thank you. |
As bad of a solution as it is, it may be better to always treat Git Gateway repos as private until #990 is in place. |
I've updated the PR to use the file contents directly (as a It works perfectly for me using GitHub and Git-Gateway backends with a public repo. @erquhart Can you test on a private repo? |
* Use file data instead of inferring path for new uploads.
- Summary
New media uploads do not appear at all until the CMS is reloaded (not the thumbnail or filename) when using the Git Gateway backend after #994. This PR resolves #1220.
- Test plan
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)