DEV-877: Split ignorer logic for Smart Build into 2 different functions according to the flag, and use a temporary file for the SHA calculation #4689
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…g is disabled Signed-off-by: Nacho Fuertes <nacho@okteto.com>
Signed-off-by: Nacho Fuertes <nacho@okteto.com>
Signed-off-by: Nacho Fuertes <nacho@okteto.com>
ifbyol
commented
Mar 24, 2025
Comment on lines
-325
to
+342
| relpath := resolveRelativePath(filepath.Join(worktree.GetRoot(), f), contextDir) | ||
| shouldIgnore, err := r.ignorer(contextDir).Ignore(relpath) | ||
| if r.shouldIgnoreFiles { | ||
| worktree, err := repo.Worktree() | ||
| if err != nil { | ||
| oktetoLog.Debugf("ignore error in GetDiffHash: %v", err) | ||
| } | ||
| if !shouldIgnore { | ||
| filteredUntrackedFiles = append(filteredUntrackedFiles, f) | ||
| oktetoLog.Debugf("Failed to get worktree, skipping calculation of files to ignore: %v", err) | ||
| } else { | ||
| oktetoLog.Debugf("skipping %v file change in GetDiffHash based on known ignore files", f) | ||
| filteredUntrackedFiles := []string{} | ||
| for _, f := range untrackedFiles { | ||
| relpath := resolveRelativePath(filepath.Join(worktree.GetRoot(), f), contextDir) | ||
| shouldIgnore, err := r.ignorer(contextDir).Ignore(relpath) | ||
| if err != nil { | ||
| oktetoLog.Debugf("ignore error in GetDiffHash: %v", err) | ||
| } | ||
| if !shouldIgnore { | ||
| filteredUntrackedFiles = append(filteredUntrackedFiles, f) | ||
| } else { | ||
| oktetoLog.Debugf("skipping %v file change in GetDiffHash based on known ignore files", f) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| untrackedFiles = filteredUntrackedFiles |
There was a problem hiding this comment.
This is the new path. It is only considered when the feature flag is enabled
pkg/repository/local_git.go
Outdated
Comment on lines
367
to
386
| f, err := lg.fs.Create(filepath.Join(config.GetOktetoHome(), fmt.Sprintf("git-untracked-%s-%d", filepath.Base(dirPath), time.Now().UnixNano()))) | ||
| if err != nil { | ||
| oktetoLog.Debugf("failed to create untracked file: %v", err) | ||
| return "", err | ||
| } | ||
|
|
||
| defer func() { | ||
| err := lg.fs.Remove(f.Name()) | ||
| if err != nil { | ||
| oktetoLog.Debugf("failed to close untracked file: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| _, err = f.WriteString(filteredLS) | ||
| if err != nil { | ||
| oktetoLog.Debugf("failed to write untracked file: %v", err) | ||
| return "", err | ||
| } | ||
|
|
||
| hashObjectCmdArgs := []string{"--no-optional-locks", "hash-object", f.Name()} |
There was a problem hiding this comment.
This is the new logic to create a temporary file with the output of the ls-files command, instead of doing a pipe (the problem with big repos is that we were exceeding the maximum length of an argument
| return handleError(err) | ||
| } | ||
| return string(output), nil | ||
| } | ||
|
|
||
| // getDirContentSHAWithoutIgnore calculates the SHA of the content of the given directory using git ls-files and git hash-object | ||
| func (lg *LocalGit) getDirContentSHAWithoutIgnore(ctx context.Context, gitPath, dirPath string, fixAttempt int) (string, error) { |
There was a problem hiding this comment.
It is the content of the function GitDirContent before the change. It can be found here
| } | ||
|
|
||
| // diffWithoutIgnore calculates the diff of the repository at the given path | ||
| func (lg *LocalGit) diffWithoutIgnore(ctx context.Context, gitPath, dirPath string, fixAttempt int) (string, error) { |
There was a problem hiding this comment.
It is the implementation of the function Diff as it was before the problematic changes. It can be found here
| ) | ||
|
|
||
| type mockLocalGit struct { |
There was a problem hiding this comment.
Even if we already have a mockLocalExec, I wasn't be able to use it as I had to mock 2 calls to RunCommand in the same test, and it wasn't allowing it. So, instead of modifying all the tests, I created another implementation using testify mocks for simplicity
Signed-off-by: Nacho Fuertes <nacho@okteto.com>
Signed-off-by: Nacho Fuertes <nacho@okteto.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4689 +/- ##
==========================================
+ Coverage 48.74% 48.79% +0.05%
==========================================
Files 354 354
Lines 29401 29468 +67
==========================================
+ Hits 14332 14380 +48
- Misses 13936 13946 +10
- Partials 1133 1142 +9 🚀 New features to boost your workflow:
|
jLopezbarb
reviewed
Mar 25, 2025
| hashObjectCmdArgs := []string{"--no-optional-locks", "hash-object", "--stdin"} | ||
| output, err := lg.exec.RunPipeCommands(ctx, gitPath, "echo", []string{filteredLS}, lg.gitPath, hashObjectCmdArgs) | ||
| // As the list of files might be too long, we write it to a file and calculate the hash of the file | ||
| f, err := lg.fs.Create(filepath.Join(config.GetOktetoHome(), fmt.Sprintf("git-untracked-%s-%d", filepath.Base(dirPath), time.Now().UnixNano()))) |
There was a problem hiding this comment.
I don't really know if I like this. I think it should be preferable to write into a temple instead of okteto home.
If we have a problem removing it if we do it in a temp file when the computer restarts it will be deleted, while if we do this in the okteto home we could be increasing the folder
There was a problem hiding this comment.
We have this proble 8000 m in other parts of the CLI like the custom kubeconfig for the deploys. We should unify and standardize how we do it, but I don't know if we should do it as part of this PR or not
Co-authored-by: Javier López Barba <javier@okteto.com>
jLopezbarb
approved these changes
Mar 25, 2025
github-actions bot
pushed a commit
that referenced
this pull request
Mar 25, 2025
…ns according to the flag, and use a temporary file for the SHA calculation (#4689) * DEV-877: Made changes to not change the default behavior when the flag is disabled Signed-off-by: Nacho Fuertes <nacho@okteto.com> * DEV-877: Fix format of the file Signed-off-by: Nacho Fuertes <nacho@okteto.com> * DEV-877: Speed up SHA calculation creating a temporary file Signed-off-by: Nacho Fuertes <nacho@okteto.com> * DEV-877: Changed mock to see if a windows unit test is fixed Signed-off-by: Nacho Fuertes <nacho@okteto.com> * DEV-877: Fix indentation of test file Signed-off-by: Nacho Fuertes <nacho@okteto.com> * Update pkg/repository/local_git.go Co-authored-by: Javier López Barba <javier@okteto.com> --------- Signed-off-by: Nacho Fuertes <nacho@okteto.com> Co-authored-by: Javier López Barba <javier@okteto.com> (cherry picked from commit 2c924ed)
ifbyol
added a commit
that referenced
this pull request
Mar 25, 2025
…ns according to the flag, and use a temporary file for the SHA calculation (#4689) (#4690) * DEV-877: Made changes to not change the default behavior when the flag is disabled Signed-off-by: Nacho Fuertes <nacho@okteto.com> * DEV-877: Fix format of the file Signed-off-by: Nacho Fuertes <nacho@okteto.com> * DEV-877: Speed up SHA calculation creating a temporary file Signed-off-by: Nacho Fuertes <nacho@okteto.com> * DEV-877: Changed mock to see if a windows unit test is fixed Signed-off-by: Nacho Fuertes <nacho@okteto.com> * DEV-877: Fix indentation of test file Signed-off-by: Nacho Fuertes <nacho@okteto.com> * Update pkg/repository/local_git.go Co-authored-by: Javier López Barba <javier@okteto.com> --------- Signed-off-by: Nacho Fuertes <nacho@okteto.com> Co-authored-by: Javier López Barba <javier@okteto.com> (cherry picked from commit 2c924ed) Co-authored-by: Ignacio Fuertes <nacho@okteto.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
After the changes introduced to ignore defined in
.dockerignorefiles in the Smart Build logics here, it turns out that for big repos with a lot of files, the command executed to calculate the SHA was failing, provoking that the image is built every time.Even if there was a feature flag for the feature, the bug comes in a common place, so any could be affected independently of enabling the feature or not.
In this PR we are doing 2 things:
git ls-objectscommand, to then, pass it to thegit hash-objectfunction, instead of doing a pipe as we were doing beforeHow to validate
There are a couple of ways for validating it.
You need to play with the environment variable
OKTETO_SMART_BUILDS_IGNORE_FILES_ENABLEDto enable and disable.When the environment variable
OKTETO_SMART_BUILDS_IGNORE_FILES_ENABLEDset to false, if you run a build using the log-leveldebugyou will see how this command is executed:/usr/bin/git --no-optional-locks ls-files -s <path-to-some-folder> | /usr/bin/git --no-optional-locks hash-object --stdin. This is to calculate the SHA of the directory.Prior to this change, you can see how the following commands are executed:
/usr/bin/git --no-optional-locks ls-files -s. To list all the files in the directoryecho <filtered output of previous command> | /usr/bin/git --no-optional-locks hash-object --stdinTo get the SHAWhen the list of files was too long, it was generating an error, so image was being built always. With this PR, if the flag is disabled, we restore the original way of calculating the SHA, as no function to ignore files has to be applied, so no need to change the commands to execute.
Then, if you change the value of the environment variable
OKTETO_SMART_BUILDS_IGNORE_FILES_ENABLEDto true, and you see the logs, you will see now how the command to be used is different, fixing the issue when the repository has too many files.You can see how now the command
/usr/bin/git --no-optional-locks ls-files -sis executed first to get the list of files, and then,/usr/bin/git --no-optional-locks hash-object <base-path>/.okteto/git-untracked-backend-1742893596755404000to calculate the SHA of the directory. It is using a temporary file to store the filtered output of thegit ls-filescommand to prevent the issue of passing a huge argument to the exec command