esnet / gdg

Grafana Dashboard Manager
https://software.es.net/gdg/
Other
347 stars 34 forks source link

[BUG] Configuration Loading issue #317

Closed efflamlemaillet closed 1 week ago

efflamlemaillet commented 2 weeks ago

I am testing gdg to backup dashboard and it works great but i found the following behavior of the option -c could be missleading:

Describe the bug Config file without yaml or yml extension passed with -c or --config are ignored without warning even if the syntax is correct. Even more the program overwrite the config at ./config/importer.yml if it exists.

To Reproduce Steps to reproduce the behavior:

  1. Generate a default config : ./gdg tools ctx list
  2. Change the current context sed -i 's/context_name: qa/context_name: testing/' config/importer.yml
  3. Copy the file to another location cp config/importer.yml conf
  4. The copy is correct but not loaded and it overwrite the config present at default location:
    #see the default config modified (context is testing)
    /app # ./gdg  tools ctx list
    ┌────────────┬────────┐
    │ CONTEXT    │ ACTIVE │
    ├────────────┼────────┤
    │ production │ false  │
    │ qa         │ false  │
    │ *testing   │ true   │
    └────────────┴────────┘
    #try to use the copy without extension 
    /app # ./gdg -c conf  tools ctx list
    2024-11-08 15:29:59 INF No configuration file has been found, creating a default configuration
    ┌────────────┬────────┐
    │ CONTEXT    │ ACTIVE │
    ├────────────┼────────┤
    │ production │ false  │
    │ *qa        │ true   │
    │ testing    │ false  │
    └────────────┴────────┘
    # The default config has been overwritten (context is back to qa)
    /app # ./gdg  tools ctx list
    ┌────────────┬────────┐
    │ CONTEXT    │ ACTIVE │
    ├────────────┼────────┤
    │ production │ false  │
    │ *qa        │ true   │
    │ testing    │ false  │
    └────────────┴────────┘

Expected behavior

  1. A valid file passed by cli argument should be used regardless of it extension Or at least if this behavior is not desired a WARN about an invalid argument should be printed. And in my opinion the program should stop if the user expected configuration is not used.

  2. The presence of default file must be checked before creating a default configuration over it

Desktop (please complete the following information):

safaci2000 commented 2 weeks ago

Okay a few minor notes.

you don't need to use sed to change context, just use: gdg tools ctx set testing.

The reason this is failing is because it can't determine the file format. Though GDG tends to mostly favor yaml, your config file can be in a number of other syntax including JSON, TOML etc. So if you give it a file with no extension, it tried to determine the format, can't and continues to do a best effort with the default config.

I think I'm going to drop the whole default config all together. It seems to cause more issues than anything else. Having a pattern to generate a working config is good, but that fall back is getting a bit confusing for users.

I'll have a PR out soon, though the rename is an obvious easy work around.

efflamlemaillet commented 2 weeks ago

I did not know about the other syntax, i understand. In my case i use gitlab-ci variables files so the file name has no extension initially. Anyway it is more about logging and documentation about 1. if there are many extension supported.

the fact that it overwrite the config/importer.yml in case of an bad file passed to -c is still a issue i think.

I agree that keeping a tool that generate a example of config is a nice feature for beginners and it also helps to migrate when syntax changes.

safaci2000 commented 1 week ago

I added a fix so that it's able to handle missing extension you ran into but it will also require a valid configuration for crud and tooling operation. It doesn't really make sense to have a default on some non existent Grafana server.

I'll polish this up and add documentation before releasing the next version.