simulot / immich-go

An alternative to the immich-CLI command that doesn't depend on nodejs installation. It tries its best for importing google photos takeout archives.
GNU Affero General Public License v3.0
1.2k stars 36 forks source link

Configuration file is always "re-written" even if it already exists #202

Open kai-tub opened 3 months ago

kai-tub commented 3 months ago

Hey, thanks for the project! :pray:

I am experiencing an issue where the configuration json file is always being written even if it already exists on disk. After a quick look, I assume that the issue is from the lines below:

https://github.com/simulot/immich-go/blob/9c624732e605e7e85f855e853346dd3c8c4f4ced/cmd/shared.go#L107-L131

It reads the configuration and later writes the configuration irrespective of whether the data was complete. If I understand correctly, the JSON file doesn't need to be updated over time.

The reason is that my secrets are stored on a read-only file system (for further reference, see LoadCredential from systemd) and that currently triggers the errors.

I believe it would be sufficient to check if the config was read and only write if no config file was read or if a new CLI argument was used. I have a draft PR/commit available if it helps:

https://github.com/simulot/immich-go/commit/3b560c18b98712c3f7ef1734f05a44614ab5592c

diff --git a/cmd/shared.go b/cmd/shared.go
index 5352eb4..29bca16 100644
--- a/cmd/shared.go
+++ b/cmd/shared.go
@@ -106,11 +106,22 @@ func (app *SharedFlags) Start(ctx context.Context) error {
    // If the client isn't yet initialized
    if app.Immich == nil {
        conf, err := configuration.Read(app.ConfigurationFile)
-       confExist := err == nil
-       if confExist && app.Server == "" && app.Key == "" && app.API == "" {
-           app.Server = conf.ServerURL
-           app.Key = conf.APIKey
-           app.API = conf.APIURL
+       confExists := err == nil
+       updateConf := !confExists
+       // cmd line args take precendence and will update config
+       if confExists {
+           if app.Server != "" || app.Key != "" || app.API != "" {
+               updateConf = true
+           }
+           if app.Server == "" {
+               app.Server = conf.ServerURL
+           }
+           if app.Key == "" {
+               app.Key = conf.APIKey
+           }
+           if app.API == "" {
+               app.API = conf.APIURL
+           }
        }

        switch {
@@ -124,15 +135,17 @@ func (app *SharedFlags) Start(ctx context.Context) error {
            return joinedErr
        }

-       // Connection details are saved into the configuration file
-       conf.ServerURL = app.Server
-       conf.APIKey = app.Key
-       conf.APIURL = app.API
-       err = conf.Write(app.ConfigurationFile)
-       if err != nil {
-           err = fmt.Errorf("can't write into the configuration file: %w", err)
-           joinedErr = errors.Join(joinedErr, err)
-           return joinedErr
+       if updateConf {
+           // Connection details are saved into the configuration file
+           conf.ServerURL = app.Server
+           conf.APIKey = app.Key
+           conf.APIURL = app.API
+           err = conf.Write(app.ConfigurationFile)
+           if err != nil {
+               err = fmt.Errorf("can't write into the configuration file: %w", err)
+               joinedErr = errors.Join(joinedErr, err)
+               return joinedErr
+           }
        }

        app.Immich, err = immich.NewImmichClient(app.Server, app.Key, app.SkipSSL)

Also happy to refactor if necessary :)

kai-tub commented 2 months ago

Ah, sorry. I never got a notification for the :+1: emoji... I have been running my own patch for some time, and I've seen https://github.com/simulot/immich-go/issues/211 has been merged in the mean time. Would you accept a PR that would split up the configuration file into individual files? My motivation is again linked to secrets being stored in a non-writeable location (the API token). But the host URL for example, is something I would like to inspect/change/update and not "hard-code" into the secret/configuration file.

But I can also understand if this is not something you are interested in. Just let me know :+1:

simulot commented 2 months ago

I'm working on a version with a better user interface. I'll use your pr