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

[CLEANUP] Migration of options to more appropriate sections #2681

Merged
merged 11 commits into from
Nov 3, 2023

Conversation

AnomalRoil
Copy link
Member

This PR migrates a few "legacy" options into new config sections, since we didn't have sectioning back when we created these. Namely:

Legacy option name New option name
core.showsafecontent show.safecontent
core.autoclip generate.autoclip
core.showautoclip show.autoclip

This also adds an easy migration path to our config handling, which should allow us to migrate option names around much more easily in the future. I've also tweaked our doc config test to detect legacy options that are still in use.

Any system level config or Env variables options are not migrated.

This also fixes a bug in our test code, where the root mount path was not properly set in our config, because we used "path:" instead of "path=" to set it in our config.

I've also spotted some wild, weird spaces which I've replaced with regular spaces and I've reformatted our markdown tables since I was playing with our documentation. I've also fixed a few typos here and there.

Since we discussed it previously in #2617, this also migrates away from using GOPASS_CONFIG_CONFIG_VALUE_n instead of GOPASS_CONFIG_VALUE_n for specifying custom options through Env variables, in order to match how GIT_CONFIG works.

NB: This is a big PR, but each commit is distinct and I'm happy opening multiple PRs if you prefer.

@AnomalRoil AnomalRoil added ux User experience / User Interface related cleanup Code hygiene labels Nov 2, 2023
This adds an easy migration path to our config handling, which should
allow us to migrate option names around much more easily in the future.

Any system level config or env variables options are not migrated.

This also fixes a bug in our test code, where the root mount path was
not properly set in our config, because we used "path:" instead of
"path=" to set it.

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
This also makes sure that legacy options aren't used in the code anymore using the docs test and its regexp

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
This is a fun one where if your Timezone isn't UTC and you are past midnight but it's not past midnight UTC, the tests would fail because you're not using the right date to validate it.

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
…he custom Env variables

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
…o GOPASS_CONFIG_KEY_i

As discussed in #2617, this actually reflects the way GIT_CONFIG works.

It also fixes a potential Panic in our codebase when IsSet was called
without any Preset config on a non-existing key.

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
@AnomalRoil AnomalRoil force-pushed the fix/optionnames branch 3 times, most recently from 0609d1c to 8229185 Compare November 2, 2023 12:07
@AnomalRoil
Copy link
Member Author

It seems our linter on CI got some new Linters because we're using latest and enable-all and so it was complaining about a lot of things, I had to add a huge commit, the second to last because of it to fix them all.

It seems we also started advertising Go1.21 in our go.mod file, but our CI was still on Go1.20, I've changed it in my last commit.

Maybe I should first do another PR with these two CI commits and then rebase this branch on top. What do you prefer @dominikschulz ?

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
@AnomalRoil AnomalRoil changed the title [CLEANUP] Moving some options to new, more correct section [CLEANUP] Moving some options to more appropriate sections Nov 2, 2023
@AnomalRoil AnomalRoil changed the title [CLEANUP] Moving some options to more appropriate sections [CLEANUP] Migration of options to more appropriate sections Nov 2, 2023
@dominikschulz dominikschulz added this to the 1.15.9 milestone Nov 2, 2023
@dominikschulz
Copy link
Member

Splitting off the two huge commits would have been nice, but now that I've made my way through this huge PR we can leave it as is.

Thanks a lot, this is a very nice improvement and cleanup effort.

dominikschulz
dominikschulz previously approved these changes Nov 2, 2023
internal/config/config.go Outdated Show resolved Hide resolved
Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
@AnomalRoil
Copy link
Member Author

I'm always very sad there isn't some kind of global git config option to sign-off commits T.T
This should now be all green hopefully.

@dominikschulz dominikschulz merged commit d56639f into master Nov 3, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code hygiene ux User experience / User Interface related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants