Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kai-tub opened this issue Mar 26, 2024 · 2 comments
Open

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

kai-tub opened this issue Mar 26, 2024 · 2 comments

Comments

@kai-tub
Copy link
Contributor

kai-tub commented Mar 26, 2024

Hey, thanks for the project! 馃檹

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:

immich-go/cmd/shared.go

Lines 107 to 131 in 9c62473

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
}
switch {
case app.Server == "" && app.API == "":
joinedErr = errors.Join(joinedErr, errors.New("missing -server, Immich server address (http://<your-ip>:2283 or https://<your-domain>)"))
case app.Server != "" && app.API != "":
joinedErr = errors.Join(joinedErr, errors.New("give either the -server or the -api option"))
}
if app.Key == "" {
joinedErr = errors.Join(joinedErr, errors.New("missing -key"))
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)

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:

3b560c1

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
Copy link
Contributor Author

kai-tub commented Apr 21, 2024

Ah, sorry. I never got a notification for the 馃憤 emoji...
I have been running my own patch for some time, and I've seen #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 馃憤

@simulot
Copy link
Owner

simulot commented Apr 21, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants