False positive warning about different source settings

Andrioden commented 1 year ago

What happened?

A complex dependency hierarchy where the same repo are downloaded multiple times output a warning repeated (probably the same amount of times the repo is downloaded by devspace). Se below. But when i check the files, they are identical. I also checked the line endings in VS Code, they are also the same.

warn Seems like you have multiple dependencies with name hn-felleslogg, but they use different source settings (C:\Users\andre\.devspace\dependencies\https-dev-azure-com-REPLACED-REPLACED-git-hn-fellesl-3697925\devspace.yaml != C:\Users\andre\.devspace\dependencies\https-REPLACED-dev-azure-com-REPLACED-REPLACED-git--9489b9f\devspace.yaml). This can lead to unexpected results and you should make sure that the devspace.yaml name is unique across your dependencies or that you use the dependencies.overrideName option

warn Seems like you have multiple dependencies with name hn-felleslogg, but they use different source settings (C:\Users\andre\.devspace\dependencies\https-dev-azure-com-REPLACED-REPLACED-git-hn-fellesl-3697925\devspace.yaml != C:\Users\andre\.devspace\dependencies\https-REPLACED-dev-azure-com-REPLACED-REPLACED-git--9489b9f\devspace.yaml). This can lead to unexpected results and you should make sure that the devspace.yaml name is unique across your dependencies or that you use the dependencies.overrideName option

warn Seems like you have multiple dependencies with name hn-felleslogg, but they use different source settings (C:\Users\andre\.devspace\dependencies\https-dev-azure-com-REPLACED-REPLACED-git-hn-fellesl-3697925\devspace.yaml != C:\Users\andre\.devspace\dependencies\https-REPLACED-dev-azure-com-REPLACED-REPLACED-git--9489b9f\devspace.yaml). This can lead to unexpected results and you should make sure that the devspace.yaml name is unique across your dependencies or that you use the dependencies.overrideName option

What did you expect to happen instead?

No warnings

How can we reproduce the bug? (as minimally and precisely as possible)

Set up a complex dependency hierarchy, and use the files below?

Left

version: v2beta1
name: hn-felleslogg

imports:
- git: https://REPLACED@dev.azure.com/REPLACED/REPLACED/_git/HN-Oppsett
  branch: master
  subPath: kubernetes/devspace/functions.yaml

dependencies:
  hn-oppsett:
    git: https://REPLACED@dev.azure.com/REPLACED/REPLACED/_git/HN-Oppsett
    branch: master
    pipeline: deploy

pipelines:
  deploy: |-
    REPLACED_default_deploy
    ensure_hosts_file_localhost_entry felleslogg-internalapi.k8s.REPLACED.utvikling
  dev: |-
    REPLACED_default_dev

deployments:
  felleslogg:
    namespace: dev
    helm:                                   
      chart: 
        name: ./helm-chart
      valuesFiles:
        - helm-chart/values.common.yaml
        - helm-chart/values.dev.yaml

dev:
  felleslogg-internalapi:
    labelSelector:
      app.kubernetes.io/name: felleslogg-internalapi
    resources:
      limits:
        memory: 2Gi
      requests:
        cpu: 2000m
        memory: 512Mi
    devImage: mcr.microsoft.com/dotnet/sdk:6.0
    env:
      - name: NUGET_REPLACED_SOURCE_PAT
        value: ${NUGET_REPLACED_SOURCE_PAT}
    sync:
    - path: ./:/app
      excludeFile: .dockerignore
      startContainer: true
    command: ["dotnet"]
    args: ["watch", "run", "--launch-profile", "Felleslogg.InternalAPI.Kubernetes", "--project", "/app/Source/Felleslogg/Felleslogg.InternalAPI/", "--non-interactive"]
    open:
    - url: http://felleslogg-internalapi.k8s.REPLACED.utvikling:30080/swagger/index.html

Right

version: v2beta1
name: hn-felleslogg

imports:
- git: https://REPLACED@dev.azure.com/REPLACED/REPLACED/_git/HN-Oppsett
  branch: master
  subPath: kubernetes/devspace/functions.yaml

dependencies:
  hn-oppsett:
    git: https://REPLACED@dev.azure.com/REPLACED/REPLACED/_git/HN-Oppsett
    branch: master
    pipeline: deploy

pipelines:
  deploy: |-
    REPLACED_default_deploy
    ensure_hosts_file_localhost_entry felleslogg-internalapi.k8s.REPLACED.utvikling
  dev: |-
    REPLACED_default_dev

deployments:
  felleslogg:
    namespace: dev
    helm:                                   
      chart: 
        name: ./helm-chart
      valuesFiles:
        - helm-chart/values.common.yaml
        - helm-chart/values.dev.yaml

dev:
  felleslogg-internalapi:
    labelSelector:
      app.kubernetes.io/name: felleslogg-internalapi
    resources:
      limits:
        memory: 2Gi
      requests:
        cpu: 2000m
        memory: 512Mi
    devImage: mcr.microsoft.com/dotnet/sdk:6.0
    env:
      - name: NUGET_REPLACED_SOURCE_PAT
        value: ${NUGET_REPLACED_SOURCE_PAT}
    sync:
    - path: ./:/app
      excludeFile: .dockerignore
      startContainer: true
    command: ["dotnet"]
    args: ["watch", "run", "--launch-profile", "Felleslogg.InternalAPI.Kubernetes", "--project", "/app/Source/Felleslogg/Felleslogg.InternalAPI/", "--non-interactive"]
    open:
    - url: http://felleslogg-internalapi.k8s.REPLACED.utvikling:30080/swagger/index.html

Here is a screenshot of them diffed in VS Code

Local Environment:

DevSpace Version: 6.2.4
Operating System: windows
ARCH of the OS: AMD64 Kubernetes Cluster:
Cloud Provider: kind (local)
Kubernetes Version: not relevant

FabianKramm commented 1 year ago

@Andrioden thanks for reporting this issue! We will investigate and fix this problem!

Andrioden commented 1 year ago

@FabianKramm - Could you look into at the same time if all dependencies are loaded, even when limiting what dependencies are used?

Se below. I get the warning 4 times.

PS C:\Projects\NHN\HN-Tjenester> devspace deploy --dependency "-"
info Using namespace 'dev'
info Using kube context 'kind-hn-cluster'
warn Seems like you have multiple dependencies with name hn-felleslogg .. [removed]
warn Seems like you have multiple dependencies with name hn-configuration, .. [removed]
warn Seems like you have multiple dependencies with name hn-felleslogg, .. [removed]
warn Seems like you have multiple dependencies with name hn-felleslogg, .. [removed]
...

lizardruss commented 1 year ago

@Andrioden Hello! I'm looking into this issue. Based on the log output, it seems that different revisions are being used for the dependency hn-felleslogg. DevSpace will consider the dependency to be different if a different revision is used somewhere in the dependency tree, even if devspace.yaml is the same in both revisions.

This is based on the output from the log:

(C:\Users\andre\.devspace\dependencies\https-dev-azure-com-REPLACED-REPLACED-git-hn-fellesl-3697925\devspace.yaml != C:\Users\andre\.devspace\dependencies\https-REPLACED-dev-azure-com-REPLACED-REPLACED-git--9489b9f\devspace.yaml)

I'd be curious if you could provide more of the dependency tree configuration where hn-felleslogg is included as a dependency. It seems like revision is used, but with different values. What doesn't make sense to me is that the second path being compared doesn't include the substringgit-hn-fellesl-, and instead has git--. Seeing how the dependency is referenced in the overall graph would help determine if there is a bug.

Andrioden commented 1 year ago

@lizardruss - I took another look, and it looks like we messed up. We are using different git urls pointing to the same repo, within the same dependency hierarchy. Se below.

I will correct it later, will report back here.

repo1

dependencies:
  hn-felleslogg:
    git: https://dev.azure.com/REPLACED/REPLACED/_git/HN-Felleslogg
    branch: master

repo2

dependencies:
  hn-felleslogg:
    git: https://REPLACED@dev.azure.com/REPLACED/REPLACED/_git/HN-Felleslogg
    branch: master

Andrioden commented 1 year ago

@lizardruss - I fixed all dependency git urls. Still have the same warning. Below is an example with hn-configuration.

-REMOVED INFO-

Notes

hn-oppsett + hn-felleslogg do not have any relevant dependencies.
The files references are identical
I tried to delete the cache in C:\Users[MYUSER].devspace\dependencies

lizardruss commented 1 year ago

Thanks for fixing the dependencies! I think this might be working as expected.

While the files might be identical, that isn't really what the message is communicating. The sources are different (or might be different) because of how they're resolved. In this case, the dependency graph has created a circle, beginning and ending with hn-configuration, but the beginning is a local workspace (C:\Projects\NHN\HN-Configuration\devspace.yaml) and the end is a cloned git repository (C:\Users\andre\.devspace\dependencies\https-nhnfelles-dev-azure-com-nhnfelles-helsenorge-git--50633e1\devspace.yaml). DevSpace isn't comparing the contents of these devspace.yaml because those files could import other configuration using imports or local manifests from deployments, both of which could have different contents. Because of that possibility, it wouldn't always be correct to consider those the same, and could lead to a confusing scenario where one might be making changes to the local workspace and expect to see those changes in the dependency without pushing the changes to the git repository.

The suggestion here would be to use the dependencies.overrideName option to modify the name to something like hn-configuration-master, which should clear the warning.

Andrioden commented 1 year ago

@lizardruss - I see.

I tested by running devspace deploy for a repo that has several circulating dependencies, but the repo are not part of the circulation it self. That worked great, so your reasoning checks out.

DevSpace isn't comparing the contents of these devspace.yaml because those files could import other configuration using imports or local manifests from

Aah, damn, good point. I see how doing a devspace.yaml string content compare is not the way. I could ofc argue, "compare all locally references files then", but thats going to be a hellish work which needs coding the understanding of how manifest, helm chart files etc relate", so yea, i wouldnt suggest this.

Wouldnt the correct behavior be that when the top devspace deployed repo (hn-configuration here) is depended on below in the dependency hierarchy, then that top devspace is assumed to be correct? And if logged at all, it should be logged as info?

The suggestion here would be to use the dependencies.overrideName option to modify the name to something like hn-configuration-master, which should clear the warning.

I cant seem to find documentation for this? Could you give an example?

lizardruss commented 1 year ago

Ah, sorry, the property is just the key used in dependencies, so you can try:

dependencies:
  ...
  hn-configuration-master:
    git: https://nhnfelles@dev.azure.com/nhnfelles/Helsenorge/_git/HN-Configuration
    branch: master

The error message is a little misleading there. Let us know if that doesn't work.

Andrioden commented 1 year ago

I see, so the devspace name and dependency name cant be the same if there is a circular dependency.

The following works

hn-configuration

version: v2beta1
name: hn-configuration-s # <- Changed to different

hn-sts

dependencies:
  hn-configuration:
    git: https://nhnfelles@dev.azure.com/nhnfelles/Helsenorge/_git/HN-Configuration
    branch: master

I am not entirely happy about this solution. But i guess you can solve the issue if you dont see any better way to design this.

Andrioden commented 1 year ago

@lizardruss - I had another take on this with a colleague. We are not completely happy with how DevSpace behave. : P

TLDR, as I see the state of the tool:

✔️ DevSpace (the tool) should have a clear opinion on how things behave/resolve
✔️ Ensure the local hn-configuration (service) is deployed singularly, priority over the dependent one
❌ Do not give a warning
❌ If warning is kept, changing the name should not affect wheter or not the warning happens

Long:

In the case described here, where i run devspace deploy from hn-configuration repo which is part of a circular chain, i expect the local devspace.yaml to be deployed to kubernetes, not any of the dependent versions of the same repo. The fact that the name: name: hn-configuration and the dependency name is equal is imo beside the point, even if the names are different i still want the local hn-configuration to be deployed, and not the dependent ones, as long as the service name is equal.

deployments:
  configuration:  # <- Service name
    helm:
      chart: 
        name: ./helm-chart
      valuesFiles:
        - helm-chart/values.common.yaml
        - helm-chart/values.dev.yaml

Changing the name to hide the warning seems like bad design, why should what the name is matter, its still the same potential problem?

DevSpace should be opinionated on which (local hn-configuration vs dependent hn-configuration) wins and are actually deployed. As per the deployment sequence, i know the local wins.

So:

The name should not matter, the warning should not happen
If you still insist it does, what do you (DevSpace tool) interpret the names to mean? They are not unique names for deployments,
Also, why would renaming either of them matter, cant devspace resolve that internally? By prefix the name internally by "local-name" (local-hn-configuration) vs "dependency-name" (dependency-hn-configuration).

Or to ask in a different way, what am I the developer telling YOU DevSpace creators by giving it a different name?

lizardruss commented 1 year ago

I talked to the team and it's intended that DevSpace should use the local copy when the names match. The warning was a mistake.

I assumed that since the dependency was defined using git, that it should use the git copy in all scenarios (a very literal interpretation of the config), and that this was to avoid issues with uncommitted local changes.

Anyway, I'll be adding tests to verify the behavior and to verify that the warning is not displayed.

devspace-sh / devspace

False positive warning about different source settings #2501