-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
New config layout #414
New config layout #414
Conversation
f92f33f
to
7ac7b53
Compare
Codecov Report
@@ Coverage Diff @@
## master #414 +/- ##
==========================================
- Coverage 63.33% 62.17% -1.16%
==========================================
Files 50 50
Lines 3060 3141 +81
==========================================
+ Hits 1938 1953 +15
- Misses 972 1034 +62
- Partials 150 154 +4
Continue to review full report at Codecov.
|
7708c93
to
d0bb006
Compare
Oh, that's a bug then. If viper.WriteConfig() doesn't write out all the config then yes, I have to use viper.WriteConfigAs(). I'll double check the behaviour. |
21c1169
to
873fec5
Compare
I don't know why codecov is commenting every 5 lines on the stuff that isn't covered by tests ... Super noisy, sorry folks I try to find out how to disable that (unless it's just me?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great already, I should have time tomorrow to weigh in more and catch up on the other stuff. Thanks for taking this on, huge improvement, and I never expected we'd migrate the original configs so that's a nice bonus
fmt.Println("Warning: Could not delete old config file", oldconfig) | ||
} | ||
|
||
// HACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, it doesn't surprise me at all that we had to do this. Should be fine since lab configs have remained simple, essentially for this reason too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why codecov is commenting every 5 lines on the stuff that isn't covered by tests ... Super noisy, sorry folks I try to find out how to disable that (unless it's just me?)
Hmm ... I'm by no means an expert in codecov. We do have a few engineers who are familiar with it. Let me see if I can ping someone to take a look.
Nicely done @prarit ! |
3dd3552
to
7aeeb5a
Compare
Switch to using TOML format for the config files as the first step of rehauling the Config setup. TOML provides better viper support and is closer to the standard git config file format. HCL format is broken in viper and requires a small hack to convert double square brackets to single square brackets in the viper TOML file format. The test code extensively uses HCL and employs several viper HCL workarounds. The test code has been modified to remove HCL code and those workarounds. Before this change the ~/.config/lab.hcl contains "core" = { "host" = "https://gitlab.com" "token" = "abcdef12345" "user" = "prarit" } After this change the ~/.config/lab/lab.toml contains [core] host = "https://gitlab.com" token = "abcdef12345" user = "prarit" Also move the new config files into their own directories in ~/.config and the local .git. Convert config files from HCL to TOML and move them into their own ~/.config/lab and .git/lab/ directories. Signed-off-by: Prarit Bhargava <prarit@redhat.com>
The existing config code uses a lot of workarounds for viper's issues with the HCL format. With the move TOML format for the config files, a lot of this code can be eliminated by using standard viper calls. Rework the config code to remove workarounds and reorganize the config function. Suggested-by: Zaq? Wiedmann <zaquestion@gmail.com> Signed-off-by: Prarit Bhargava <prarit@redaht.com>
Move the config initialization code from main.go to internal/config/config.go. Suggested-by: Bruno Meneguele <bmeneg@redhat.com> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
As noted in issue zaquestion#152 ("Accept self signed certificates"), the certificate code is no longer working. This is because the certificate code was executed before the configuration was loaded. The code is need of a massive clean up, and with the transition of the config files to TOML format, the code to get the certificate can easily be incorporated into the main configuration code. Fix and clean up the certifcate code. Signed-off-by: Prarit Bhargava <prarit@redhat.com>
7aeeb5a
to
51d9b7d
Compare
@zaquestion, I've dropped WIP from the description and the patches. This code is ready for review IMO. |
I'll be doing a deep dive over the weekend, thanks @prarit ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT the changes are working as expected and the code is pretty sane and clean.
Thanks @prarit .
Looking solid, I took it for a spin locally and saw my configs convert seamlessly. I added a simple test for the convert func, after the tests pass lets move forward and bring this in. |
note: confpath convert was reordered to happen first to mitigate a very unlikely edge case where lab is run in `.config` causing the converted toml file to land in `.config` instead of `.config/lab/`
5b2f9c1
to
6d8163f
Compare
@prarit I'll let you do the honors 👍 |
When running lab in GitLab CI, lab will attempt to autoconfigure to use the CI credentials. While this is a useful feature for consumers of lab, it's working against us here. I've used the lab env vars which currently take precedence over the CI config, but when #414 comes in, I believe it'll break again, so we'll need to revisit then. The ssh config make it so lab can clone repos over ssh, as that key is attached to the `lab-testing` user
WIP: New config layout