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

JS module for uploading and fetching object URLs in Linode Object Storage #1

Merged
merged 1 commit into from
Jan 7, 2022

Conversation

Ushmajit
Copy link
Contributor

@Ushmajit Ushmajit commented Jan 7, 2022

Tested the module by writing a sample node starter file and then resuing the module.

Expected behavior verified :

  1. Verified that file is being uploaded successfully into the bucket.
  2. Verified object Url is properly fetched using Linode V4 APIs

@Ushmajit Ushmajit requested a review from abose January 7, 2022 10:10
@abose abose merged commit 938f01d into main Jan 7, 2022
"node-fetch": "^3.0.0",
"path": "^0.12.7",
"url-join": "^4.0.1",
"winston": "^3.3.3"
Copy link
Member

Choose a reason for hiding this comment

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

unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad removed unused lib Winston.

},
"author": "ushmajit",
"type": "module",
"license": "ISC"
Copy link
Member

Choose a reason for hiding this comment

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

GNU AGPL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

* @param bucket uniquely identifies the bucket where the file should be uploaded
* @returns {Promise<void>}
*/
async function uploadFileToLinodeBucket (accessKeyId, secretAccessKey, region, file, bucket) {
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have some predefined constants for regions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, makes sense, created an issue to track this #3

@abose abose added the Under Review Pull requests that are under review label Jan 9, 2022
@abose
Copy link
Member

abose commented Jan 9, 2022

Unit test notes:
While we are starting to choose JS unit test frameworks, just some notes

  • Jest is a good framework for unit tests. It has straightforward integration to test react/angular/stuff and is maintained by FB.
  • Mocha is used by brackets/Phoenix, so it might be good to have mocha for uniform test framework across our repos. Also has been around for a decade and is widely used.

@abose
Copy link
Member

abose commented Jan 9, 2022

  • https://github.com/aicore/template-nodejs-ts is a template repo that can be used to quickly create nodejs project scaffolding. But it is still in development 📦
  • If there is something that is built in this lib that can be shared to the template, consider adding it there too for future pulls.
  • Also there are some issue/pr template files that are in the template but needs to be copied to this repo.

@abose
Copy link
Member

abose commented Jan 9, 2022

Done. I have updated https://github.com/aicore/template-nodejs with best practices we follow for brackets and phoenix.

  • Integrated unit and integration test frameworks
  • Integrated static code analytics tools
  • github actions, git commit taps etc.

Just copy and merge the contents of the repo here to get all the functions. Follow readme. :)

@Ushmajit
Copy link
Contributor Author

Thanks for the template files, copied all the relevant files from the template to the module. This is the new PR #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Under Review Pull requests that are under review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants