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

DEV-335 Repository is not being calculated correctly for Smart Builds when the manifest is within a subdirectory of a repository #4307

Merged
merged 4 commits into from
May 24, 2024

Conversation

ifbyol
Copy link
Member

@ifbyol ifbyol commented May 21, 2024

Proposed changes

Fixes DEV-335

When calculating the git repository for Smart Builds we were not checking parent folders to get git metadata and extract the remote URL. This was generating that for example, if you have the following structure:

root/
  |- subdirA/
         |- okteto.yml

And you execute okteto deploy from subdirA, image were being rebuilt every time because we don't extract git metadata correctly.

Also, the filename stored in the configmap wasn't right because we were storing empty or okteto.yml instead of storing the file relative to the root of the repo, so redeploy or destroy from the UI are not working correctly. Currently, we just store the filename if it was specified by the user using -f flag.

In other part of the code, we were calculating the repository checking parent folders, but in a different way, so I modified both places to use the same logic. In this case, we use the property DetectDotGit and pass it to git.PlainOpenWithOptions.

Starting to calculate the git repository generates that the dev environment name inferred might be different, as the git repository has preference over the folder name.

How to validate

Execute deploys and build operations in different scenarios:

  • From the root of a repo having the manifest in the root without specifying it with -f (no filename is stored in the configmap)
  • From the root of a repo having the manifest in the root specifying it with -f
  • From the root of a repo having the manifest in a subfolder specifying it with -f
  • From the root of a repo having a compose in the root without specifying it with -f (no filename is stored in the configmap)
  • From the root of a repo having a compose in the root specifying it with -f
  • From the root of a repo having a compose in a subfolder specifying it with -f
  • From a subfolder of a repo having the manifest in the same folder without specifying it with -f (no filename is stored in the configmap)
  • From a subfolder of a repo having the manifest in the same folder specifying it with -f
  • From a subfolder of a repo having the manifest in a subfolder specifying it with -f
  • From a subfolder of a repo having a compose in the same folder without specifying it with -f (no filename is stored in the configmap)
  • From a subfolder of a repo having a compose in the same folder specifying it with -f
  • From a subfolder of a repo having a compose in a subfolder specifying it with -f
  • From a subfolder of a repo having a manifest in the root specifying it with -f
  • From a subfolder of a repo having a compose in the root specifying it with -f
  • From a folder with no repository having a manifest
  • From a folder with no repository having a compose

For all the cases with repositories, make sure that redeploy works fine. Also, check that the filename stored in the configmap is the correct one when using the -f option (otherwise, the filename is not stored)

CLI Quality Reminders 🔧

For both authors and reviewers:

  • Scrutinize for potential regressions
  • Ensure key automated tests are in place
  • Build the CLI and test using the validation steps
  • Assess Developer Experience impact (log messages, performances, etc)
  • If too broad, consider breaking into smaller PRs
  • Adhere to our code style and code review guidelines

… when the manifest is within a subdirectory of a repository. The filename stored in the configmap wasn't correct either (not relative to the repo root)

Signed-off-by: Nacho Fuertes <[email protected]>
@ifbyol ifbyol added release/bug-fix run-e2e-windows When used on a PR run windows e2e tests run-e2e When used on a PR run windows & unix e2e run-e2e-unix When used on a PR run unix e2e labels May 21, 2024
@ifbyol ifbyol requested a review from a team as a code owner May 21, 2024 16:11
"annotation2": "value2",
"annotation1": "value1",
"annotation2": "value2",
"dev.okteto.com/sample": "true",
Copy link
Member Author

Choose a reason for hiding this comment

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

This (and the rest of changes in this file) is needed because, as the tests run within okteto/okteto repository, it is being detected and added the annotation because it is considered an Okteto sample. I tried to change it, but there is no easy way to do it as it uses current working directory internally

@@ -1153,8 +1157,10 @@ func Test_getStackName(t *testing.T) {

for _, tt := range tests {
t.Run(tt.testName, func(t *testing.T) {
dir := t.TempDir()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed to prevent the test to detect okteto/okteto repository

Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 31.81818% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 43.05%. Comparing base (19eb936) to head (8ac172b).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4307      +/-   ##
==========================================
- Coverage   43.10%   43.05%   -0.05%     
==========================================
  Files         371      371              
  Lines       30021    30045      +24     
==========================================
- Hits        12940    12936       -4     
- Misses      15954    15975      +21     
- Partials     1127     1134       +7     

…est path and manifest path flag instead of the path within the manifest object. Added more test cases in the E2E test

Signed-off-by: Nacho Fuertes <[email protected]>
…removed unsued params in functions

Signed-off-by: Nacho Fuertes <[email protected]>
@ifbyol ifbyol merged commit 72a7823 into master May 24, 2024
12 of 14 checks passed
@ifbyol ifbyol deleted the ifbyol/fix-repo-calculation branch May 24, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/bug-fix run-e2e When used on a PR run windows & unix e2e run-e2e-unix When used on a PR run unix e2e run-e2e-windows When used on a PR run windows e2e tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants