DiffyWebsite / ddev-diffy

Diffy integration with DDEV
https://diffy.website
Apache License 2.0
1 stars 0 forks source link

Global variables do not get populated to .env if start from scratch #5

Closed ygerasimov closed 1 month ago

ygerasimov commented 1 month ago

Steps to reproduce:

But instead they both should be populated with values you provided during installation of the add-on.

Not that if you keep variables in your .ddev/config.yaml and ~/.ddev/global_config.yaml files and reinstall the add-on (ddev get --remove diffywebsite/ddev-diffy && ddev get diffywebsite/ddev-diffy) you will get variables values populated in diffy-worker/.env file.

Video demo https://www.loom.com/share/bff2d15d3278431393415d763e656d6a

rfay commented 1 month ago

You seem to be putting them into the DDEV global configuration in https://github.com/DiffyWebsite/ddev-diffy/blob/main/install.yaml#L15-L43

Those populate the web container.

I guess you're then trying to set them for the diffy container in https://github.com/DiffyWebsite/ddev-diffy/blob/e49bb8c01ba6eff88d2d29496e81643f373b2c9b/install.yaml#L87-L88

You seem to be using {{ $foundDiffAPIKey }} in https://github.com/DiffyWebsite/ddev-diffy/blob/main/install.yaml#L61 (probably a typo anyway, should be $foundDiffyAPIKey ?) but I don't see where that has ever been set.

I also don't really understand why you ever put those into the global config instead of just putting them into the diffy .env to begin with.

rfay commented 1 month ago

Oh, I see super-complicated

    DIFFY_API_KEY='{{- $foundDiffApiKey := false -}}
    {{- range .DdevGlobalConfig.web_environment }}
      {{- if not $foundDiffApiKey }}
        {{- $keyVal := splitList "=" . }}
        {{- if eq (index $keyVal 0) "DIFFY_API_KEY" }}
          {{- index $keyVal 1 -}}
          {{- $foundDiffApiKey = true -}}
        {{- end }}
      {{- end }}
    {{- end }}'

That's probably a bad idea (and apparently isn't working)

ygerasimov commented 1 month ago

Worker code we would love to keep outside of ddev-diffy repo as it is used elsewhere in the system (like on our actual production workers).

API Key is a variable to store in global configuration as you might have multiple ddev projects using it. So user doesn't need to re-enter it over and over again.

@rfay Does it make sense to simplify all this by asking users simply to add values to .env file directly? That will be the most straight forward solution.

rfay commented 1 month ago

I think the problem you're having with this approach is that you're counting on .DdevGlobalConfig.web_environment`, but DDEV read the ~/.ddev/global_config.yaml, etc, before starting the installation, and you then changed it to add the info. You'll need to use a different technique to get the information.

rfay commented 1 month ago

Think about why you're populating the global config and web config anyway. Will it be used in the web container?

ygerasimov commented 1 month ago

It won't be used in web container at all.

rfay commented 1 month ago

Seems like a mistake to be adding it as environment variables at all then... Instead of doing that, add it to the .env to begin with? You could check the .env instead of checking the global and project config, then add it there?

ygerasimov commented 1 month ago

ok. Let me do that instead. Thank you. I'll make it much simpler.

rfay commented 1 month ago

BTW, thanks for all your exploration here. I'm quite sure ddev-platformsh can be improved since DDEV has evolved and has more capabilities than it did when it was originally done. Maybe we can improve things there, especially using a .env, which is better than editing global config, etc.

rfay commented 1 month ago

Interestingly enough, DDEV already loads godotenv and could use it in the future to read or (write?) arbitrary .env files.

ygerasimov commented 1 month ago

@rfay I have simplified the addon. I am not asking for the credentials during installation and have instructions to add .env file manually. This will keep things straightforward.