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

Remove S3 File Adapter #7324

Merged
merged 3 commits into from
Apr 9, 2021
Merged

Conversation

mtrezza
Copy link
Member

@mtrezza mtrezza commented Apr 9, 2021

This removes the S3 Files Adapter from the Parse Server repo.

Instead, the adapter has to be installed separately as dependency @parse/s3-files-adapter.
This is to not make Parse Server dependent on the update of an optional adapter.
The same has already been done some time ago with GCS Files Adapter.

Incident for this change:

  • The S3 files adapter was reported to have a vulnerability, therefore also Parse Server was reported to have that vulnerability. That is factually incorrect for deployments that do not use the adapter.

@codecov
Copy link

codecov bot commented Apr 9, 2021

Codecov Report

Merging #7324 (47aa2ef) into master (bf732b9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 47aa2ef differs from pull request most recent head 10f8ded. Consider uploading reports for the commit 10f8ded to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7324   +/-   ##
=======================================
  Coverage   93.88%   93.89%           
=======================================
  Files         181      181           
  Lines       13194    13194           
=======================================
+ Hits        12387    12388    +1     
+ Misses        807      806    -1     
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)
src/RestWrite.js 93.92% <0.00%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf732b9...10f8ded. Read the comment docs.

@dplewis
Copy link
Member

dplewis commented Apr 9, 2021

Just want to point out that the docker build is broken

Cloning into '.'...
Warning: Permanently added the RSA host key for IP address '140.82.114.3' to the list of known hosts.
Reset branch 'master'
Your branch is up-to-date with 'origin/master'.
Pulling cache layers for index.docker.io/parseplatform/parse-server:latest...
Done!
KernelVersion: 4.4.0-1060-aws
Components: [{u'Version': u'19.03.8', u'Name': u'Engine', u'Details': {u'KernelVersion': u'4.4.0-1060-aws', u'Os': u'linux', u'BuildTime': u'2020-03-11T01:24:30.000000000+00:00', u'ApiVersion': u'1.40', u'MinAPIVersion': u'1.12', u'GitCommit': u'afacb8b7f0', u'Arch': u'amd64', u'Experimental': u'false', u'GoVersion': u'go1.12.17'}}, {u'Version': u'1.2.13', u'Name': u'containerd', u'Details': {u'GitCommit': u'7ad184331fa3e55e52b890ea95e65ba581ae3429'}}, {u'Version': u'1.0.0-rc10', u'Name': u'runc', u'Details': {u'GitCommit': u'dc9208a3303feef5b3839f4323d9beb36df0a9dd'}}, {u'Version': u'0.18.0', u'Name': u'docker-init', u'Details': {u'GitCommit': u'fec3683'}}]
Arch: amd64
BuildTime: 2020-03-11T01:24:30.000000000+00:00
ApiVersion: 1.40
Platform: {u'Name': u'Docker Engine - Community'}
Version: 19.03.8
MinAPIVersion: 1.12
GitCommit: afacb8b7f0
Os: linux
GoVersion: go1.12.17
Starting build of index.docker.io/parseplatform/parse-server:latest...
Step 1/22 : FROM node:lts-alpine as build
---> 645c08831ecc
Step 2/22 : RUN apk update; apk add git;
---> Running in bc002f625f2f
fetch http://dl-cdn.alpinelinux.org/alpine/v3.11/main/x86_64/APKINDEX.tar.gz
fetch http://dl-cdn.alpinelinux.org/alpine/v3.11/community/x86_64/APKINDEX.tar.gz
v3.11.10-8-gf8ac3999ba [http://dl-cdn.alpinelinux.org/alpine/v3.11/main]
v3.11.10-2-gc655f5d2f5 [http://dl-cdn.alpinelinux.org/alpine/v3.11/community]
OK: 11274 distinct packages available
(1/6) Installing ca-certificates (20191127-r2)
(2/6) Installing nghttp2-libs (1.40.0-r1)
(3/6) Installing libcurl (7.67.0-r3)
(4/6) Installing expat (2.2.9-r1)
(5/6) Installing pcre2 (10.34-r1)
(6/6) Installing git (2.24.4-r0)
Executing busybox-1.31.1-r10.trigger
Executing ca-certificates-20191127-r2.trigger
OK: 23 MiB in 22 packages
Removing intermediate container bc002f625f2f
---> e3d351ac090d
Step 3/22 : WORKDIR /tmp
---> Running in 3f8bbba68f56
Removing intermediate container 3f8bbba68f56
---> b052cf9a8a1d
Step 4/22 : COPY package*.json ./
---> 5cb2da3f79c8
Step 5/22 : RUN npm ci
---> Running in 0f3c176d9412
�[91mnpm�[0m
�[91m ERR! code ENOENT
�[0m
�[91mnpm ERR! �[0m
�[91msyscall open
�[0m
�[91mnpm ERR! path /tmp/node_modules/mock-mail-adapter/package.json
�[0m
�[91mnpm ERR! �[0m
�[91merrno -2
�[0m
�[91mnpm ERR! enoent�[0m
�[91m ENOENT: no such file or directory, open '/tmp/node_modules/mock-mail-adapter/package.json'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! �[0m
�[91menoent
�[0m
�[91m
�[0m
�[91mnpm ERR! A complete log of this run can be found in:
npm ERR! /root/.npm/_logs/2021-04-08T22_53_48_782Z-debug.log
�[0m
Removing intermediate container 0f3c176d9412
The command '/bin/sh -c npm ci' returned a non-zero code: 254

dplewis
dplewis previously approved these changes Apr 9, 2021
Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

LGTM!

@dplewis dplewis dismissed their stale review April 9, 2021 01:30

broken build

@mtrezza
Copy link
Member Author

mtrezza commented Apr 9, 2021

@dplewis Docker build is broken? Was that only temporary?

@dplewis
Copy link
Member

dplewis commented Apr 9, 2021

The latest build / branch is broken. Caused by #7321 and this PR may cause another similar error.

Edit: In package.json

"mock-mail-adapter": "file:spec/support/MockMailAdapter",
"mock-files-adapter": "file:spec/support/MockFilesAdapter",

@mtrezza
Copy link
Member Author

mtrezza commented Apr 9, 2021

That's because of the local dependency I guess, I already thought that was too easy...

"mock-mail-adapter": "file:spec/support/MockMailAdapter",
"mock-files-adapter": "file:spec/support/MockFilesAdapter",

Any idea how to fix this for docker?
Where should the path actually point to if not /tmp/node_modules/mock-mail-adapter?

Maybe adding a ./ will already fix this, can you try?

"mock-mail-adapter": "file:./spec/support/MockMailAdapter",
"mock-files-adapter": "file:./spec/support/MockFilesAdapter",

@mtrezza
Copy link
Member Author

mtrezza commented Apr 9, 2021

@dplewis I changed the path in this PR, could you see if that works for docker?

@dplewis
Copy link
Member

dplewis commented Apr 9, 2021

@davimacedo I haven't used docker in while. Can you give it a try?

You could try COPY ./spec /spec in the build stage

Perhaps we should add testing DockerFile in the Contributions?

@mtrezza
Copy link
Member Author

mtrezza commented Apr 9, 2021

I added a docker build check to the CI in #7332; I suggest we add the fix there, because it also contains the check.

@mtrezza mtrezza requested a review from dplewis April 9, 2021 15:00
@dplewis dplewis merged commit 2e11bf3 into parse-community:master Apr 9, 2021
@mtrezza mtrezza deleted the remove-s3-adapter branch April 9, 2021 15:12
Arul- pushed a commit to Arul-/parse-server that referenced this pull request Apr 12, 2021
* remove s3 adapter

* moved mock files adapter

* Update package-lock.json
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@mtrezza mtrezza mentioned this pull request Mar 12, 2022
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants