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

Support Array of ParseFiles #65

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

codebreach
Copy link

fixes #35

@codecov-io
Copy link

Current coverage is 13.69% (diff: 4.28%)

Merging #65 into master will decrease coverage by 1.89%

@@             master        #65   diff @@
==========================================
  Files             4          4          
  Lines           263        314    +51   
  Methods          64         71     +7   
  Messages          0          0          
  Branches         77         95    +18   
==========================================
+ Hits             41         43     +2   
- Misses          222        271    +49   
  Partials          0          0          

Powered by Codecov. Last update 43726a4...14b325d

@codebreach
Copy link
Author

I doubt this will get merged but this works fine in case anyone needs to do some last minute migrations.

just two things to keep in mind:

  • make sure "transferTo": 's3' is set in config.js otherwise your files will not have url set
  • make sure parse-server-s3-adapter has directAccess: true otherwise the files uploaded will not have public access. (basically this should match your setting in parse-server.

Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

Thanks for that feature, it's looking great! A few things to address though!

@@ -131,7 +131,7 @@ function _processFiles(files, handler) {
var asyncLimit = config.asyncLimit || 5;
return new Promise(function(resolve, reject) {
async.eachOfLimit(files, asyncLimit, function(file, index, callback) {
process.stdout.write('Processing '+(index+1)+'/'+files.length+'\r');
process.stdout.write('Processing '+(index+1)+'/'+files.length+'\r');
Copy link
Contributor

Choose a reason for hiding this comment

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

oops!

console.log("Fetching schema...");
return schemas.get().then(function(res){
console.log("Fetching all objects with files...");
var schemasWithFiles = onlyFiles(res);
if (config.onlyFiles){
Copy link
Contributor

Choose a reason for hiding this comment

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

could we do this feature 'additive' instead of file or array? Like just files, or files + arrays?

if (config.transferTo === 'filesystem'){
var _path = config.filesAdapter._filesDir + "/" + file.newFileName;
} else if (config.transferTo === 's3'){
var _path = "https://" + config.filesAdapter._bucket + "." + config.filesAdapter._s3Client.config.endpoint + "/" + config.filesAdapter._bucketPrefix + file.newFileName;
Copy link
Contributor

Choose a reason for hiding this comment

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

there should be getFileLocation https://github.com/parse-server-modules/parse-server-s3-adapter/blob/master/index.js#L122 available from the S3 adapter, can we use that instead?

@codebreach
Copy link
Author

@flovilmart Thanks for the review
I do not have access to Ilhasoft:only-string-and-array. I just found it fixing the issue so decided to open a PR for visibility. I don't have my own fork and thus cannot make the changes.

Maybe @teehamaral who owns that repo and wrote this code originally can swoop in?

@flovilmart
Copy link
Contributor

Or you can fork on your repo from the Ilhasoft repo?

@teehamaral
Copy link

@codebreach I could make the changes, but I don't have available time now, maybe we can work together to fix it.

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.

4 participants