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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle no topgrade.toml but files in topgrade.d #460

Merged
merged 1 commit into from
Jun 13, 2023
Merged

fix: handle no topgrade.toml but files in topgrade.d #460

merged 1 commit into from
Jun 13, 2023

Conversation

slowsage
Copy link
Contributor

@slowsage slowsage commented Jun 2, 2023

When config file(s) exist in topgrade.d but no topgrade.toml exists, ensure() does not create a default config file. This causes an error as read() assumes the topgrade.toml file exists.

This PR assumes the desired behavior is to use just the contents of the files in topgrade.d.

Else, one could change ensure() to create topgrade.toml.

Standards checklist:

  • The PR title is descriptive.
  • I have read CONTRIBUTING.md
  • The code compiles (cargo build)
  • The code passes rustfmt (cargo fmt)
  • The code passes clippy (cargo clippy)
  • The code passes tests (cargo test)
  • Optional: I have tested the code myself
    • I also tested that Topgrade skips the step where needed

If you developed a feature or a bug fix for someone else and you do not have the
means to test it, please tag this person here.

@SteveLauC
Copy link
Member

SteveLauC commented Jun 2, 2023

Good catch! I can reproduce the issue that this PR tries to fix with the latest git version of topgrade, and tested that this PR will fix it by using the files under topgrade.d

$ pwd
/home/steve/.config/topgrade.d


$ ls
Permissions Links Size User  Group Date Modified Name
.rw-r--r--@     1  191 steve steve  2 Jun 14:59  topgrade.toml

$ topgrade --dry-run 2>&1 | head -n 2
ERROR Unable to read
ERROR failed to load configuration: No such file or directory (os error 2)
[1]    155988 IOT instruction (core dumped)  topgrade --dry-run 2>&1 |
       155989 done                           head -n 2

I don't have a strong view on how we should handle it under such a case (using files under config.d or creating a default config file), but honestly, I don't like the idea of topgrade.d, IMHO, it really makes the whole thing complicated, topgrade does not have so many configuration entries that it needs to disperse the configuration files across the file system. And if users do want to separate their configuration files into different files, the [include] section is sufficient.

Friendly ping @DottoDev , any thoughts on this?

@slowsage
Copy link
Contributor Author

slowsage commented Jun 2, 2023

Note on topgrade.d: Another complication (and possible future issue) is the the [misc] section. Currently, it is required in the main topgrade.toml file but not in the files in topgrade.d.

@s34m
Copy link
Member

s34m commented Jun 2, 2023

IMHO there should be the possibility to use topgrade.d but by default it should automatically use and/or create thetopgrade.toml file at $HOME/.config/

@slowsage
Copy link
Contributor Author

slowsage commented Jun 3, 2023

IMHO there should be the possibility to use topgrade.d but by default it should automatically use and/or create thetopgrade.toml file at $HOME/.config/

Should [misc] be required in the files in topgrade.d?

@s34m
Copy link
Member

s34m commented Jun 5, 2023

Best case would be that [misc] isn't required.

@slowsage
Copy link
Contributor Author

slowsage commented Jun 10, 2023

@DottoDev , to confirm:

  1. [misc] required in main file (as is currently)
  2. Do not process/require [misc] for files in topgrade.d (as is currently)
  3. Create topgrade.toml if it doesn't exist even if there are files in topgrade.d

This way the topgrade.toml file can be the only one to have [misc], [include]. Avoiding [include] in topgrade.d avoids some recursive [include] cases.

Please lmk if that makes sense, I'll change the diff to address 3. I'll leave 1. and 2. as is.

@s34m
Copy link
Member

s34m commented Jun 11, 2023

@slowsage seems good, but I don't know if topgrade.toml has to be required if topgrade.d exists, imo it doesn't have to be created in this case.

@slowsage
Copy link
Contributor Author

@DottoDev , this PR handles exactly that case. i.e no topgrade.toml file => use defaults and apply files in topgrade.d if they exist. No file created. Please accept if it makes sense.

@s34m
Copy link
Member

s34m commented Jun 13, 2023

Lgtm

@s34m s34m merged commit 3e6c6e5 into topgrade-rs:master Jun 13, 2023
7 checks passed
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

Successfully merging this pull request may close these issues.

None yet

3 participants