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

Feat: use params for get operation #63

Merged
merged 2 commits into from Mar 9, 2019
Merged

Feat: use params for get operation #63

merged 2 commits into from Mar 9, 2019

Conversation

florianbepunkt
Copy link
Contributor

Summary

(If you have not already please refer to the contributing guideline as described
here
)

  • Tell us about the problem your pull request is solving.
    Currently you can set params.s3 object that will passed to the create op. For example this allows to specify an S3 Bucket where the blob should be uploaded to. But the get operation does not respect the params. So you can store a blob in a bucket you specify but you cannot retreive it.
  • Are there any open issues that are related to this?
  • Is this PR dependent on PRs in other repos?
    No

If so, please mention them to keep the conversations linked together.

Other Information

If there's anything else that's important and relevant to your pull
request, mention that information here. This could include
benchmarks, or other information.

Your PR will be reviewed by a core team member and they will work with you to get your changes merged in a timely manner. If merged your PR will automatically be added to the changelog in the next release.

If your changes involve documentation updates please mention that and link the appropriate PR in feathers-docs.

Thanks for contributing to Feathers! ❤️

@florianbepunkt
Copy link
Contributor Author

Regarding tests… currently there are no params test, which makes sense since they would be adapter specific. But to demonstrate the basic use case:

it('Get: Retreives file from specified bucket', async () => {
    const params = {
      s3: {
        Bucket: 'my-non-default-bucket'
      }
    };

    const service = app.service('/file');

    const blob = {
      id: 'hello-world-different-bucket.txt',
      uri: getBase64DataURI(
        Buffer.from(new String('this is a test')),
        'text/plain'
      )
    };

    await service.create(blob, params);

    await expect(service.get('hello-world-different-bucket.txt', params)).to.be
      .fulfilled;
  });

@florianbepunkt
Copy link
Contributor Author

Not sure why tests are failing… seems to be an issue with the test config not the pr.

@daffl
Copy link
Member

daffl commented Mar 3, 2019

This makes sense, thank you! Tests are failing because external PRs don't have access to the S3 test credentials, it should be passing once merged.

If you would like to add the test you were mentioning, you should be able to run the tests locally (with a normal npm test) by setting the AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY and S3_BUCKET environment variables.

@florianbepunkt
Copy link
Contributor Author

@daffl Shall I add a the test to this repo?

@daffl daffl merged commit 9e29056 into feathersjs-ecosystem:master Mar 9, 2019
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.

None yet

2 participants