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

@uppy/companion: fix Dockerfile and deploy automation #3355

Merged
merged 6 commits into from Dec 9, 2021

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Dec 9, 2021

Because I realised that we're not using PnP, I attempted fixing dockerfile so that it works with yarn workspaces and respects yarn.lock

Alternative to #3347

had to set hoisting limit for companion so that all node_modules are kept inside companion folder, I think this makes sense in general because companion is kind of on its own

i tested the docker file and it launches successfully

that works with yarn workspaces and respects yarn.lock
@mifi mifi requested a review from aduh95 December 9, 2021 13:49
Copy link
Member

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

You'd need to change those lines as well

context: packages/@uppy/companion
file: packages/@uppy/companion/Dockerfile
.

It's a bit unfortunate that the Docker files for Companion is no longer in the @uppy/companion folder, but if that's all it needs, that's an acceptable tradeoff.

Dockerfile Outdated

RUN apk --update add --virtual native-dep \
make gcc g++ python3 libgcc libstdc++ git && \
cd /app && corepack yarn plugin import workspace-tools && \
Copy link
Member

@aduh95 aduh95 Dec 9, 2021

Choose a reason for hiding this comment

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

The workspace-tools plugin should be already in /app/.yarn, can we remove this instruction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops i missed that,thanks!

@mifi
Copy link
Contributor Author

mifi commented Dec 9, 2021

Yea, it might be possible to make it work with Dockerfile inside packages/@uppy/companion too, but I'm not enough of a docker guru to fix that. It would have to include files from parent directories and also work with docker-compose.yml.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 changed the title Dockerfile yarn berry @uppy/companion: fix Dockerfile and deploy automation Dec 9, 2021
@kiloreux
Copy link
Contributor

kiloreux commented Dec 9, 2021

Are you moving the Dockerfile outside of companion package folder? Why? 😅

@mifi
Copy link
Contributor Author

mifi commented Dec 9, 2021

I couldn't find a way to run yarn install inside docker from the root of the monorepo (where yarn.lock resides) without setting the docker working directory to the root of the monorepo. if you know how, then please let me know :) can we simply put context: ../../.. inside docker-compose.yml and it will just work?

@aduh95 aduh95 merged commit bac107f into main Dec 9, 2021
@aduh95 aduh95 deleted the dockerfile-yarn-berry branch December 9, 2021 17:12
@aduh95
Copy link
Member

aduh95 commented Dec 9, 2021

So the Docker build job completed successfully, but the Heroku one failed: https://github.com/transloadit/uppy/runs/4473760891?check_suite_focus=true...

@github-actions github-actions bot mentioned this pull request Dec 9, 2021
github-actions bot added a commit that referenced this pull request Dec 9, 2021
| Package           | Version | Package           | Version |
| ----------------- | ------- | ----------------- | ------- |
| @uppy/angular     |   0.2.7 | @uppy/store-redux |   2.0.3 |
| @uppy/audio       |   0.2.1 | @uppy/svelte      |   1.0.6 |
| @uppy/aws-s3      |   2.0.7 | @uppy/vue         |   0.4.4 |
| @uppy/companion   |   3.1.3 | @uppy/xhr-upload  |   2.0.7 |
| @uppy/core        |   2.1.4 | @uppy/robodog     |   2.1.5 |
| @uppy/dashboard   |   2.1.3 | uppy              |   2.3.1 |
| @uppy/locales     |   2.0.5 |                   |         |

- meta: update npm deps (Antoine du Hamel / #3352)
- @uppy/companion: fix Dockerfile and deploy automation (Mikael Finstad / #3355)
- @uppy/companion: don’t pin Yarn version in `package.json` (Antoine du Hamel / #3347)
- @uppy/aws-s3,@uppy/core,@uppy/dashboard,@uppy/store-redux,@uppy/xhr-upload: deps: use `nanoid/non-secure` to workaround react-native limitation (Antoine du Hamel / #3350)
- @uppy/audio: showRecordingLength option was removed, always clearInterval (Artur Paikin / #3351)
- meta: drop `stringify-object` dependency to generate locales (Antoine du Hamel / #3344)
- meta: add release automations (Antoine du Hamel / #3304)
@mifi mifi mentioned this pull request Dec 11, 2021
2 tasks
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
* attempt at fixing dockerfile

that works with yarn workspaces and respects yarn.lock

* run corepack yarn

* update Dockerfile references

* remove unneccesary yarn plugin import

* Apply suggestions from code review

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
| Package           | Version | Package           | Version |
| ----------------- | ------- | ----------------- | ------- |
| @uppy/angular     |   0.2.7 | @uppy/store-redux |   2.0.3 |
| @uppy/audio       |   0.2.1 | @uppy/svelte      |   1.0.6 |
| @uppy/aws-s3      |   2.0.7 | @uppy/vue         |   0.4.4 |
| @uppy/companion   |   3.1.3 | @uppy/xhr-upload  |   2.0.7 |
| @uppy/core        |   2.1.4 | @uppy/robodog     |   2.1.5 |
| @uppy/dashboard   |   2.1.3 | uppy              |   2.3.1 |
| @uppy/locales     |   2.0.5 |                   |         |

- meta: update npm deps (Antoine du Hamel / transloadit#3352)
- @uppy/companion: fix Dockerfile and deploy automation (Mikael Finstad / transloadit#3355)
- @uppy/companion: don’t pin Yarn version in `package.json` (Antoine du Hamel / transloadit#3347)
- @uppy/aws-s3,@uppy/core,@uppy/dashboard,@uppy/store-redux,@uppy/xhr-upload: deps: use `nanoid/non-secure` to workaround react-native limitation (Antoine du Hamel / transloadit#3350)
- @uppy/audio: showRecordingLength option was removed, always clearInterval (Artur Paikin / transloadit#3351)
- meta: drop `stringify-object` dependency to generate locales (Antoine du Hamel / transloadit#3344)
- meta: add release automations (Antoine du Hamel / transloadit#3304)
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

4 participants