dokku-letsencrypt icon indicating copy to clipboard operation
dokku-letsencrypt copied to clipboard

Refactor: properties instead of environment variables

Open pushrbx opened this issue 3 years ago • 6 comments

This PR introduces the set sub command which you will need to use to set certain configuration options instead of using the dokku config:set command. I'm creating this as per @josegonzalez request on #257 .

pushrbx avatar May 15 '22 14:05 pushrbx

@josegonzalez Should all config-get bits be replaced with the property-get? Is there a distinguishing trait which I need to take in account when deciding what will be property and what remains as a global env var/config? E.g. the email can be global and app specific too, but from here it's clear that there can't be global properties. Can the email be a property?

pushrbx avatar May 15 '22 14:05 pushrbx

Yeah basically. You can pass --global instead of the app name for global properties.

josegonzalez avatar May 15 '22 14:05 josegonzalez

But I can't do that I think, because of the following in properties.go:

func CommandPropertySet(pluginName, appName, property, value string, properties map[string]string, globalProperties map[string]bool) {
	if appName != "--global" {
		if err := VerifyAppName(appName); err != nil {
			LogFailWithError(err)
		}
	}
	if appName == "--global" && !globalProperties[property] {
		LogFail("Property cannot be specified globally")
	}

Or am I misinterpreting the !globalProperties[property] bit?

pushrbx avatar May 15 '22 14:05 pushrbx

OK I've tested it, now I know how it works. 😸 image

pushrbx avatar May 15 '22 14:05 pushrbx

I've tested this in a VM/Ubuntu 20.04 with the following steps:

  1. create the example app from the getting started documentation of dokku. (ruby-getting-started)
  2. ran dokku letsencrypt:enable ruby-getting-started and it ran without issues.

Only one problem left which I can't figure out. In the standard output you can see the following message: ! Unable to read letsencrypt property --global.

pushrbx avatar May 29 '22 10:05 pushrbx

Any update on this merge?

TriStarGod avatar Sep 16 '22 20:09 TriStarGod

Now I've synced this with master branch, and I've applied the recommendations. Sorry for the delay on this. 😸

pushrbx avatar Oct 27 '22 17:10 pushrbx

@pushrbx I ran with your changes and did a somewhat major refactor in #294. Thanks for doing the majority of the hard parts and getting this going!

josegonzalez avatar Jan 28 '23 08:01 josegonzalez