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

Three related problems in buildDockerImageContextTar() #183

Closed
jrobbins-LiveData opened this issue Apr 22, 2021 · 4 comments
Closed

Three related problems in buildDockerImageContextTar() #183

jrobbins-LiveData opened this issue Apr 22, 2021 · 4 comments
Labels
pinned r/image Relates to the image resource r/registry_image

Comments

@jrobbins-LiveData
Copy link

In this function:

func buildDockerImageContextTar(buildContext string) (string, error) {

this line:

header.Name = strings.TrimPrefix(strings.Replace(file, buildContext, "", -1), string(filepath.Separator))

there are three related problems. These problems prevent correct execution of docker build via the "build block" inside resource "docker_registry_image".

Problem 1 (subfolder blanking)

The Replace call's last parameter is -1. Perhaps this is assuming that the buildContext will only appear once in the string, as the top-level folder? But if you have subfolders of that top-level context folder, with the same name as that top-level folder's, as the Go doc says "there is no limit on the number of replacements."

I think that last parameter to Replace should be 1 instead of -1, to only replace the first occurence of buildContext. Otherwise, a subfolder with the same name as the context folder gets represented in the tar file as an empty string. For example, my context folder is named "app". You can see what happens to a subfolder under "app" also named "app":

subfolder_named_app

Problem 2 (filename corruption)

You can also see in that image above what happens to a file named appConfigExtension.zip. The "app" part of its name is removed! This of course breaks the Dockerfile, which is expecting to see the file named appConfigExtension.zip.

Problem 3 (blank object in tar file)

The .Walk callback gets passed the context folder as the first callback file. The Replace call has the effect of renaming header.Name to the empty string, which gets placed in the tar file.

Placing this code after the Replace would prevent this problem

		if header.Name == "" {
			return nil
		}

blank_string_in_tar_file

@mavogel mavogel added r/image Relates to the image resource r/registry_image labels May 10, 2021
@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 7 days.
If you don't want this issue to be closed, please set the label pinned.

@github-actions github-actions bot added the stale label Jul 10, 2021
@jrobbins-LiveData
Copy link
Author

jrobbins-LiveData commented Jul 10, 2021

I am concerned that this issue has been marked stale without any feedback or triage. I had read this article and thought it meant that this project was being actively maintained. [email protected], what is the status of Kreuzwerker's support of this project?

@mavogel mavogel added pinned and removed stale labels Jul 11, 2021
@mavogel
Copy link
Contributor

mavogel commented Jul 11, 2021

Hi @jrobbins-LiveData this project is actively maintained. The stale bot marked this issue due to no activity. I marked it as pinned. We recently tackled problems in #240, #246 and IMHO the solution would be to reuse code from the docker cli, see #146.

Does the issue occur only on windows (assuming from the screenshots) or unix systems as well?

@Junkern
Copy link
Contributor

Junkern commented Jul 11, 2022

@jrobbins-LiveData just to give an update: We just released v2.18.0 which got rid of using the buildDockerImageContextTar() in the context of building the docker_registry_image resource (and thus got rid of quite a few issues)

It is still needed in calcuation the state-hash of the context property. My gut feeling tells me that any changes to to the state calculation (meaning to the buildDockerImageContextTar() function) of the context property will probably be a breaking change => a v3 is needed.

And if we will run into a breaking change anyway, we might as well get rid of the whole buildDockerImageContextTar() function.

In short: the usage of this function is deprecated internally and we cannot change anything in this function => your suggestions will not make it into the code and it only makes sense to close this issue

Disclaimer: I still need to verify all off the things written above. It might very well be that my assumptions are wrong, then I'll reopen this issue and work our suggestions in.

What do you think?

@Junkern Junkern closed this as completed Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned r/image Relates to the image resource r/registry_image
Projects
None yet
Development

No branches or pull requests

3 participants