Repository navigation
Fix git protocol and refactor Config interface - #8246
Conversation
64eeb8f to
30a6653
Compare
samcoe
changed the title
| require.ErrorAs(t, err, &keyNotFoundError) | ||
| } | ||
|
|
||
| func TestGitProtocolNotLoggedInDefaults(t *testing.T) { |
There was a problem hiding this comment.
This test is no longer relevant as GitProtocol no longer lives on authCfg.
|
|
||
| // When we get the git protocol | ||
| gitProtocol := authCfg.GitProtocol("github.com") | ||
| protocol, err := authCfg.cfg.Get([]string{hosts, "github.com", gitProtocol}) |
There was a problem hiding this comment.
This is kind of annoying as authCfg does not have anyway to access the git protocol now. Only used in this test so I think it is okay for now.
| Browser(string) string | ||
| Editor(string) string | ||
| GitProtocol(string) string | ||
| HTTPUnixSocket(string) string | ||
| Pager(string) string | ||
| Prompt(string) string |
There was a problem hiding this comment.
This is the big change, adding in these accessors.
|
|
||
| // GitProtocol will retrieve the git protocol for the logged in user at the given hostname. | ||
| // If none is set it will return the default value. | ||
| func (c *AuthConfig) GitProtocol(hostname string) string { |
There was a problem hiding this comment.
This is now moved off of AuthConfig and onto Config.
There was a problem hiding this comment.
Is the problem with this old code that it didn't return a final default value if an error happens?
I'm having trouble tracing this back to a specific issue to make sure I understand the problem being fixed 🙇
| // If the encrypt option is specified it will first try to store the auth token | ||
| // in encrypted storage and will fall back to the plain text config file. | ||
| func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secureStorage bool) (bool, error) { | ||
| func (c *AuthConfig) Login(hostname, username, token, protocol string, secureStorage bool) (bool, error) { |
There was a problem hiding this comment.
Changed the argument name to not conflict with the new const gitProtocol.
There was a problem hiding this comment.
It might be useful in the future to suffix these constants like gitProtocolSetting, gitProtocolConfig, gitProtocolKey if need to avoid conflicts comes up again.
There was a problem hiding this comment.
Yeah this is a good call out, I will make that change.
There was a problem hiding this comment.
Not sure how this got stale, but when I did go generate this was updated so I included it in this PR. It is unrelated to the config changes.
andyfeller
left a comment
andyfeller
left a comment
There was a problem hiding this comment.
All in all, I don't see anything glaring about these changes that has me concerned. The desire to decouple application logic from how the underlying configuration is handled makes sense, however I don't know where this tight coupling was causing problems much less how it manifested with the git protocol.
Having an issue link would help give me confidence in being able to ship these changes 🙇
| // If the encrypt option is specified it will first try to store the auth token | ||
| // in encrypted storage and will fall back to the plain text config file. | ||
| func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secureStorage bool) (bool, error) { | ||
| func (c *AuthConfig) Login(hostname, username, token, protocol string, secureStorage bool) (bool, error) { |
There was a problem hiding this comment.
It might be useful in the future to suffix these constants like gitProtocolSetting, gitProtocolConfig, gitProtocolKey if need to avoid conflicts comes up again.
|
|
||
| // GitProtocol will retrieve the git protocol for the logged in user at the given hostname. | ||
| // If none is set it will return the default value. | ||
| func (c *AuthConfig) GitProtocol(hostname string) string { |
There was a problem hiding this comment.
Is the problem with this old code that it didn't return a final default value if an error happens?
I'm having trouble tracing this back to a specific issue to make sure I understand the problem being fixed 🙇
|
@andyfeller Apologies for not expanding my PR body further. It was the end of my day and was just trying to get this open for review. For more context I opened up #8250. |
Preach! 🙌 Communication is hard on top of actually fixing issues, keep that 💩 up |
3 participants
Fixes #8250
This PR refactors the
Configinterface to now include direct accessor functions for the default configuration keys. This allows callers to not have to know about key names and hides that implementation detail from them. Outside of theconfigcommand and tests there should be no places aware of the configuration keys. As part of this work theAuthConfig.GitProtocolfunction got moved to now live onConfig. I also added in more constants in theconfigpackage for the configuration keys so that we can stop writing them out without the compilers help. This PR is not as long as it looks as most of these changes are just updating call points. Lastly, there was a stale prompter mock that is included in the PR but is unrelated to the changes above.