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

Read dockerignore in dev and deploy commands #757

Merged
merged 3 commits into from
Feb 6, 2019
Merged

Conversation

krhubert
Copy link
Contributor

close #736

Copy link
Contributor

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

Good stuff! Maybe we can reduce complexity a bit. I'd like to keep ReadDockerignore & TrimBuildFilesFromExcludes but other funcs we use from Docker's pks can be replaced with simpler ones. (I briefly reviewed the implementation of that extra funcs you put, they seem to do some OS specific stuff, resolve symlinks etc. and this is too much. I think we don't need them for now.)

We can just use this code here too, by moving it under x pkg. And also add this TrimBuildFilesFromExcludes to that func.

@krhubert
Copy link
Contributor Author

krhubert commented Feb 1, 2019

But what is the resaon behind it? Why do you want just subset of this funcionalities?

If we want to reduce complexity then we already did. We have used functions that are already writter and working. If we want to copy them, then we need to write, test, think what part cut off etc... For example why do you want to not handle symlink (or any other funcinality)?

I think we must be as most compatible with docker build as we can. Because if you are developer and you run docker build and mesg service deploy you expect the same results.

And about dockerignoreFiles I wrote it before I knew it that there is already such function. What we should do is replace it with the one from build package to avoid dedup function.

// And canonicalize dockerfile name to a platform-independent one
relDockerfile = archive.CanonicalTarNameForPath(relDockerfile)
excludes = build.TrimBuildFilesFromExcludes(excludes, relDockerfile, false)

archive, err := archive.TarWithOptions(path, &archive.TarOptions{
Copy link
Member

Choose a reason for hiding this comment

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

What about xarchive package?

Should we refacto most of the content of deployServiceSendServiceContext or simply delete xarchive package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some other packages like service (or container) can use it, but it needs refactoring first. And here is small PR to add this to dev/deploy command

Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

Why putting too much logic on the cli. A lot of these validations are great but would make more sense on the core itself.

I would go with something simple and maybe move the validation on the backend

excludes, err := build.ReadDockerignore(path)
archive, err := xarchive.GzippedTar(path, excludes)

@NicolasMahe
Copy link
Member

Why putting too much logic on the cli. A lot of these validations are great but would make more sense on the core itself.

I would go with something simple and maybe move the validation on the backend

excludes, err := build.ReadDockerignore(path)
archive, err := xarchive.GzippedTar(path, excludes)

I agree on at least try to refactor this function. But I also agree to merge this one now if @krhubert works directly on the refactor version.

@krhubert
Copy link
Contributor Author

krhubert commented Feb 4, 2019

Those functions won't work on server side and all must be used on cli where the service directory exists.

build.ValidateContextDirectory(contextDir, excludes)
ValidateContextDirectory checks if all the contents of the directory can be read and returns an error if some files can't be read symlinks which point to non-existing files don't trigger an error

archive.CanonicalTarNameForPath
It fixes the filename for diffrent os (for now we don't support windows but when we will already start with bug)

build.TrimBuildFilesFromExcludes
TrimBuildFilesFromExcludes removes the named Dockerfile and .dockerignore from the list of excluded files. (otherwise Dockerfile and .dockerignore won't be send to server)

@antho1404
Copy link
Member

I don't understand why they will not work on the server side, we are getting the tar and extracting the content so we will have the same informations that the cli has.

But anyway, same this can be done in another PR, this one can be merged

@NicolasMahe
Copy link
Member

I don't understand why they will not work on the server side, we are getting the tar and extracting the content so we will have the same informations that the cli has.

But anyway, same this can be done in another PR, this one can be merged

@antho1404 can you approve in this case

@antho1404 antho1404 merged commit b29e524 into dev Feb 6, 2019
@antho1404 antho1404 deleted the fix/dockerignore branch February 6, 2019 11:38
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.

Support dockerignore in command deploy
4 participants