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: docker #759

Merged
merged 67 commits into from
Feb 15, 2023
Merged

feat: docker #759

merged 67 commits into from
Feb 15, 2023

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Feb 14, 2023

🚥 Fixes RM-6538

🧰 Changes

This is a tiny PR to get our GitHub Action using Docker instead of a composite action. It takes substantially longer to install/build than the composite action approach in its current form because it's rebuilding the container with every workflow run, but I'll get the container registry publishing going in a separate PR (once this and #746 are in next) so we can really speed things up 🚀

🧬 QA & Testing

Do our GitHub Actions dry runs continue to work as expected?

kanadgupta and others added 30 commits February 10, 2023 13:43
apparently semantic-release likes this better I don't know
technically a chore but making it a feat to see what semantic-release does with it
# [8.6.0-next.2](v8.6.0-next.1...v8.6.0-next.2) (2023-02-10)

### Features

* add git + changelog plugins ([85e4bfd](85e4bfd))

[skip ci]
## What's Changed in 8.6.0-next.3

### Bug Fixes

* rebuild prior to npm publish ([29b9ec6](29b9ec6))
* reformat github release header ([38c5625](38c5625))

[skip ci]
## What's Changed

### Bug Fixes

* reformat header again ([bd2e1a2](bd2e1a2))

### Features

* drop duplicative tag ([4c34207](4c34207))

[skip ci]
hopefully this will put the extra tag on the correct commit?
## What's Changed

### Bug Fixes

* try rearranging steps like this ([cac0c1d](cac0c1d))

### Reverts

* Revert "feat: drop duplicative tag" ([f9fe6c6](f9fe6c6))

[skip ci]
@kanadgupta kanadgupta added the enhancement New feature or request label Feb 15, 2023
Dockerfile Outdated
@@ -0,0 +1,7 @@
FROM node:16

COPY . /rdme-docker
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what the best practices are regarding directory stuff like this... feedback appreciated 😭

Copy link
Contributor

Choose a reason for hiding this comment

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

It varies a lot AFAIK. I've seen /app before sometimes, that may be what we use in our main app. You could use /rdme if you want.

@kanadgupta kanadgupta marked this pull request as ready for review February 15, 2023 00:44
Copy link
Member

@domharrington domharrington left a comment

Choose a reason for hiding this comment

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

It takes substantially longer to install/build than the composite action approach in its current form because it's rebuilding the container with every workflow run

So will this actually be faster when we start using the registry? We should probably get that working e2e before committing to this approach just to make sure the benefits actually outweigh it here? It's not like this is a complicated project, and npm install can't take that long. Where as node:16 which is the base image we're using here is a not-exactly-lean 340MB:

$ docker pull node:16

$ docker images --filter=reference="node:16"
REPOSITORY   TAG       IMAGE ID       CREATED         SIZE
node         16        ce3d204ff857   2 minutes ago   340MB

I dont know either way which approach is better, but would be interested to prove it out more to ensure benefits. We could also investigate using one of the leaner docker images like 16-alpine, which is a slightly more palatable 42.3MB:

$ docker images --filter=reference="node:16-alpine"
REPOSITORY   TAG         IMAGE ID       CREATED          SIZE
node         16-alpine   1621ddffc775   39 seconds ago   42.3MB

Base automatically changed from semantic-release to next February 15, 2023 15:28
@kanadgupta
Copy link
Member Author

We should probably get that working e2e before committing to this approach just to make sure the benefits actually outweigh it here?

Yep, that's the point of the prerelease pipeline in #746 🙃 and didn't know about node:16-alpine, will look into that, thanks!

@domharrington
Copy link
Member

Sorry I didnt know about that! Hoping it all speeds up nicely 🚀

@kanadgupta kanadgupta merged commit ef63371 into next Feb 15, 2023
@kanadgupta kanadgupta deleted the docker branch February 15, 2023 15:50
github-actions bot pushed a commit that referenced this pull request Feb 15, 2023
# [8.6.0-next.1](v8.5.0...v8.6.0-next.1) (2023-02-15)

### Features

* docker ([#759](#759)) ([ef63371](ef63371))
* empty commit to trigger release ([2c459d8](2c459d8))

[skip ci]
github-actions bot pushed a commit that referenced this pull request Feb 15, 2023
# [8.6.0-next.1](v8.5.0...v8.6.0-next.1) (2023-02-15)

### Features

* docker ([#759](#759)) ([ef63371](ef63371))
* empty commit to trigger release ([2c459d8](2c459d8))

[skip ci]
@kanadgupta kanadgupta restored the docker branch February 15, 2023 17:58
@kanadgupta kanadgupta mentioned this pull request Feb 15, 2023
@kanadgupta kanadgupta deleted the docker branch February 15, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants