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

Sync map can be inferred from artifact Dockerfile #1166

Closed
r2d4 opened this issue Oct 15, 2018 · 4 comments · Fixed by #2088
Closed

Sync map can be inferred from artifact Dockerfile #1166

r2d4 opened this issue Oct 15, 2018 · 4 comments · Fixed by #2088

Comments

@r2d4
Copy link
Contributor

r2d4 commented Oct 15, 2018

We already parse the source in a dockerfile's COPY and ADD, but we should also capture the destination.

FROM python:3.7-slim
CMD ["python", "-m", "flask", "run"]
ENV FLASK_DEBUG=1
ENV FLASK_APP=app.py

COPY requirements.txt .
RUN pip install -r requirements.txt
COPY *.py . # Should parse dst here

So that instead of writing this:

  - image: gcr.io/k8s-skaffold/python-reload
    context: python
    sync:
      '*.py': .

Users could write this

  - image: gcr.io/k8s-skaffold/python-reload
    context: python
    sync:
      '*.py':

However, there are many corner cases to watch out for. COPY can take multiple arguments to copy to the same destination folder, and there are rules of when the destination must be inferred as a directory or not.

Overlapping copies may be another corner case. Ultimately I think it will need to be modeled as a map of source files to possibly multiple locations.

Different artifact types can also provide different hints on how to fill in the sync map.

@corneliusweig
Copy link
Contributor

@r2d4 For docker, this is a pretty good idea. But what about the other artifact types here? I guess the whole concept of sync does not really make sense for jib, but what about bazel? At the moment, a sync can be configured for all artifact types, therefore we would also need a way to infer file destinations for bazel. Or should image sync for artifacts other than docker be disallowed?

@r2d4
Copy link
Contributor Author

r2d4 commented Feb 11, 2019

Sync works for all artifact types actually. Internally each builder provides a dependency map of files for the watch controller. Docker does that by parsing the dockerfile, bazel runs a bazel query, and I’m not sure what the jib implementation is.

Sync actually only works on the files in that list anyways. The syntax has much room for improvement though.

@corneliusweig
Copy link
Contributor

Sure, sync works for all artifact types. Sorry for being so unclear about this here.
What I meant to say is that your suggested way to infer the destination path for the files to sync only works for Dockerfiles and not for the other artifact types. I had a short glance if the other artifact types do support something comparable, but I couldn't find anything. Therefore your suggested destination inference needs to be restricted to the docker artifacts.

Maybe this is too detailed now, but I also have a question about the tradeoffs for an implementation.

  • One option would be to change the signature of DependenciesForArtifact to return something map-like instead of paths []string. But then jib/bazel would unnecessarily return a complicated structure instead of the string slice. So I don't quite like that.
  • Another option would be to build the destination map only when required. But this means that the Dockerfile will be parsed a second time, meaning another scan of the working directory. Would that be acceptable?

@r2d4
Copy link
Contributor Author

r2d4 commented Feb 11, 2019

  • One option would be to change the signature of DependenciesForArtifact to return something map-like instead of paths []string. But then jib/bazel would unnecessarily return a complicated structure instead of the string slice. So I don't quite like that.

The new signature would be DependenciesForArtifact -> map[string][]string. I think thats reasonable information to ask from the builder implementations.

  • Jib could easily return a dependency map (I think it just maps certain directories to a prefixed directory in the container) @coollog WDYT? Jib should be able to tell us where the jar gets put.

  • Bazel, not so sure... DependenciesForArtifact support isn't even fully correct.

The degenerate case could be to just assume that the source matches the destination. It does seem like there's some a little too much magic going on though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants