basecamp / kamal

Deploy web apps anywhere.
https://kamal-deploy.org
MIT License
10.6k stars 407 forks source link

Getting an error when using a destination with no corresponding deploy yml file #809

Open jeromedalbert opened 4 months ago

jeromedalbert commented 4 months ago

When using -d destination, at the moment you have to have a deploy.destination.yml file (along with a deploy.yml file), or you get a ERROR (RuntimeError): Configuration file not found error.

Conversely, when using -d destination, at the moment you have to have a deploy.yml file (along with a deploy.destination.yml file), or you also get a ERROR (RuntimeError): Configuration file not found error.

I don't think it should error out. I think only one yml file can be enough, because you can make a deploy.yml file that works for all destinations.

Like this one:

service: myapp
image: myuser/myapp

servers:
  web:
  - <%= ENV['SERVER_IP'] %>

registry:
  username: myuser
  password:
    - KAMAL_REGISTRY_PASSWORD

env:
  secret:
    - RAILS_MASTER_KEY
    - SERVER_IP

Because the SERVER_IP comes from an environment variable, like .env.production or .env.staging, I don't have anything to add in a deploy.production.yml file or deploy.staging.yml file. One deploy.yml file is enough, but this won't work in Kamal at the moment.

The only way to work around this is to create a dummy deploy.production.yml file with a dummy key in it (or {} as mentioned in https://github.com/basecamp/kamal/issues/682). If the file is empty, you get a ERROR (NoMethodError): undefined methodsymbolize_keys' for false` error) error.

If this issue makes sense I’d be happy to make a PR to fix this.

igor-alexandrov commented 4 months ago

@djmb this seems to be reasonable. If you don't have anything against it, I will take care of this.

djmb commented 4 months ago

@igor-alexandrov - I'm not sure about this. The problem I see is if someone uses deploy.yml for production deployments and then adds a new destination but forgets the destination yml file, then they'd deploy to the production servers rather than get an error.

That said this is a valid use case for a single deploy file and it would be nice to avoid unnecessary duplication, but I think we need to leave this for now. At some point I'm going to look through how config inheritance works between deploy.yml and deploy.abc.yml so I'll keep this in mind when I get to that.

igor-alexandrov commented 4 months ago

@djmb what you've said also makes sense.

I am not sure what you mean about the inheritance. For me it is a pretty simple deep_merge! https://github.com/basecamp/kamal/blob/main/lib/kamal/configuration.rb#L23.

djmb commented 4 months ago

@igor-alexandrov - regarding inheritance, there may be some things we don't want to carry across from deploy.yml into deploy.abc.yml that we currently do carry - e.g. the servers. But not something I'm sure about or have specific plans for.

Right now though I think allowing a single deploy.yml might make it harder to change things later if needed.

igor-alexandrov commented 4 months ago

I got it, Donal. Thank you for the clarification.