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

CI/Dockerfiles: minor dockerfiles improvements #722

Merged
merged 5 commits into from
Dec 7, 2022

Conversation

dcaba
Copy link
Contributor

@dcaba dcaba commented Dec 2, 2022

  • CI: suggesting to execute the PR test flow also on push events (useful while developing? At least it was for me)
  • CI: executing a dumb docker-build for each Dockerfile to ensure we can generate images / there are no syntax issues or other base-image changes (we are pointing to latest!) that make break RUN statements.
  • Dockerfiles: consolidating RUN stages to reduce the number of images
  • Dockerfiles: cleaning apt cache after installing software, to save some space / make resulting image a bit slimmer, but..
  • for the rust Dockerfile: caching the crates index to save time when building C@E apps (so bye bye disk savings)
  • Dockerfiles: Consistent usage of curl --silent to avoid verbose docker-build output
  • Dockerfiles: Removing bash-specific syntax that was generating errors in the output and probably breaking checksum validations
  • Dockerfiles: Replacing adduser by useradd to avoid errors in the output (as adduser is expected to be interactive)

dcaba added 4 commits December 2, 2022 18:23
…he crates.io index to save compute apps build time. Improving docker build output. Consolidating layers by minimizing RUN stages
.github/workflows/pr_test.yml Outdated Show resolved Hide resolved
USER fastly
# this forces the download of the crates index; it will save build time if the image was recently built
RUN cargo search --limit 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Outside the concern of this PR but just thinking we should really have a Dockerfile-go as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1!! Our cli depends on the specific-programming-language build tooling, and I dont think that a common image bringing all languages tooling is going to scale well (will make the image even more huge). What we may want to do is to consolidate all Dockerfiles in a single one: a common stage can just install the fastly-cli, and then we can have dedicated stages per language we are supporting, each stage picking the common fastly-cli and adding the language-specific tooling (or inheriting it from the right base image). Then, rather than doing a docker-build against a specific Dockerfile, CI and users will need to be explicit on the stage we want to build (the common fastly-cli layers will be reused if you build more than one image/language, which is good for caching)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dcaba Nice. I bow to your knowledge in this area 🙇🏻 (ps, PRs welcome 😛)

@Integralist Integralist merged commit b682e17 into fastly:main Dec 7, 2022
@Integralist Integralist added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Dec 15, 2022
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.

2 participants