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

Copy pubspec.lock file #4

Closed
mit-mit opened this issue Apr 26, 2021 · 9 comments
Closed

Copy pubspec.lock file #4

mit-mit opened this issue Apr 26, 2021 · 9 comments

Comments

@mit-mit
Copy link
Member

mit-mit commented Apr 26, 2021

Opening to track a codereview comment by @jonasfj:

We probably should copy in both pubspec.yaml and pubspec.lock.

Assuming the idea is to make a layer that can be cached and reuse when rebuilding the image.

@athomas pointed out that the old image did: ONBUILD ADD pubspec.* /app/

@mit-mit mit-mit mentioned this issue Apr 26, 2021
@athomas
Copy link
Member

athomas commented Apr 26, 2021

We need to handle the cases where there is a pubspec.lock (checked-in or generated from a local run of pub) and where there isn't (e.g. on build system that just checks out the code and immediately runs docker build).

@subfuzion
Copy link
Collaborator

@jonasfj's comment referred to the example for the Dart Functions Framework. I think we chose originally not to copy pubspec.lock since you write a library function lib/functions.dart and then we generate the app with a builder. For the canonical structure of a function app, see the examples as well as the templates for the dartfn command. However, since ultimately we are building an app, I think @jonasfj is probably right and should revisit this decision -- building an app should be deterministic for the user who likely does want to pin dependencies to what has been tested; what do you think, @kevmoo?

Regarding using ONBUILD, we want to avoid this for an official image. The official images maintainers provide a rationale for this and suggest instead providing an example Dockerfile in the published docs. In the case of the Dart Functions Framework, we generate this Dockerfile when the function app is generated using dartfn. We might want to also update stagehand to do this automatically (or with a --dockerfile option).

@athomas
Copy link
Member

athomas commented Apr 26, 2021

Re ONBUILD: we're all in agreement on this. Nobody is arguing for ONBUILD it just happens that the old images used this.

I think production apps would use lockfiles. Otherwise, your deployment may pick up a different dependency resolution from what passed testing just because someone published a new, potentially broken, version of one of your dependencies.

@mit-mit
Copy link
Member Author

mit-mit commented Apr 26, 2021

Yes, for apps the recommendation is to checkin .lock files:
https://dart.dev/guides/libraries/private-files#pubspeclock

@subfuzion
Copy link
Collaborator

This is probably what we want:

COPY pubspec.* /app/
RUN dart pub get
COPY . .

@subfuzion
Copy link
Collaborator

subfuzion commented Apr 26, 2021

Sounds like we all agree that apps should have the .lock file copied to the docker build context. Again, will just note the original reason for not doing that for the functions framework example was because the user was only supposed to write a function under lib. Once we moved to the builder model for generating a full app (generating bin/server.dart) that is bundled together and copied into the Dockerfile, we should have also ensured that the generated .lock file was copied as well. I'll open an issue in the Functions Framework repo so we can fix this.

@athomas
Copy link
Member

athomas commented Apr 26, 2021

COPY pubspec.* /app/

Because app is the workdir, is that the same as COPY pubspec.* ./?

@subfuzion
Copy link
Collaborator

Yes. 👍

@athomas
Copy link
Member

athomas commented May 31, 2021

The examples for the official image use COPY pubspec.* ./. So as far as this repo is concerned, the issue can be closed. Please re-open if you disagree.

@athomas athomas closed this as completed May 31, 2021
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 a pull request may close this issue.

3 participants