Write per-host config info to `hosts.yml` instead of `config.yml` by mislav · Pull Request #1077 · cli/cli

mislav · 2020-06-02T13:52:30Z

All per-host configuration info, including authentication credentials, is now written to hosts.yml instead of to config.yml. This is to make config.yml safe to check into version control and share. The migration from old to new config style is done whenever new configuration is written to disk.

The biggest functional change that I've done as part of this PR is that accessing the Config object does not trigger authentication process anymore if the config file is missing. Instead, a blank Config object is returned. This is to facilitate reading configuration such as aliases #970 without triggering any side-effects.

Authentication is now triggered whenever context.AuthToken() is accessed, but when there is no oauth_token for the current hostname.

Another change in this PR is that the config implementation does not hardcode github.com anymore and handles well when other hostnames are written to and read from. This is mainly to pave the way to Enterprise compatibility #273.

Fixes #937

It's not being depended on anywhere right now, so this minimizes the public interface.

Previously we would trigger OAuth flow when the config file did not exist. Now we will let an empty Config object be initialized in that case, but trigger OAuth flow when the Context caller requests an AuthToken.

We dynamically add hosts on `Set`, so this `hosts` cache might fall out of date. We could ensure to keep it updated, but I'm not convinced it's necessary for speed right now.

Read from and write to the `hosts.yml` file every time `config.yml` is accessed. Everything that before went under the `hosts:` map now belongs to `hosts.yml`.

mislav · 2020-06-02T13:58:32Z

internal/config/config_file.go

+	var hosts map[string][]map[string]string
+	err = yaml.Unmarshal(b, &hosts)


migrateConfig was simplified to write to a new blank config and have all internal YAML- or write-related logic handled by fileConfig implementation

mislav · 2020-06-02T14:05:38Z

internal/config/config_setup.go

-	// TODO this sucks. It precludes us laying out a nice config with comments and such.
-	type yamlConfig struct {
-		Hosts map[string]map[string]string
-	}
-
-	yamlHosts := map[string]map[string]string{}
-	yamlHosts[oauthHost] = map[string]string{}
-	yamlHosts[oauthHost]["user"] = userLogin
-	yamlHosts[oauthHost]["oauth_token"] = token


🔥🔥🔥

mislav · 2020-06-02T14:06:46Z

internal/config/config_type.go

+		if nodes[i].Value == "hosts" {
+			hostsData.Content = append(hostsData.Content, nodes[i+1].Content...)
+		} else {
+			mainData.Content = append(mainData.Content, nodes[i], nodes[i+1])


Splitting config into config.yml and hosts.yml is done at Write time

vilmibm

I like how this came out and just had one minor nitpick.

We should keep in mind as a team that until users re-authenticate or set a config value, their configs will not be split. I think that's ok for now but it's something that anyone responding to support issues is going to be want to be aware of.

vilmibm · 2020-06-02T21:29:24Z

internal/config/config_setup.go

+	return token, nil
+}
+
+func AuthFlow(oauthHost, notice string) (string, string, error) {


should this still be its own function? or at least made non-exported? I only see it used in AuthFlowWithConfig

Good call! I'll make it private.

probablycorey

The biggest functional change that I've done as part of this PR is that accessing the Config object does not trigger authentication process anymore if the config file is missing.

Nice!

This works great.

probablycorey · 2020-06-02T20:45:47Z

internal/config/config_file.go

+		return err
+	}
+
+	cfgFile, err := os.OpenFile(fn, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600) // cargo coded from setup


I thought fn was a function for a bit and was very confused. Maybe filename or just f?

mislav · 2020-06-03T11:42:25Z

We should keep in mind as a team that until users re-authenticate or set a config value, their configs will not be split.

@vilmibm Do you think that we should split it immediately on read when we detect "legacy" format like we did with migrateConfig in the past? I don't think making that change would be hard, but for now I've opted to not do it since I didn't feel that there is a strong reason to do the split until the user interacts with gh config set or gh alias or re-authentication

vilmibm · 2020-06-03T22:33:59Z

Do you think that we should split it immediately on read when we detect "legacy" format like we did with migrateConfig in the past?

I'm fine with not splitting immediately; I was just pointing out that whatever we do we (ie the people that field support requests via issues) should all be aware of it.

OJFord · 2020-09-18T19:52:01Z

Thanks very much for that comment @vilmibm - I was initially disappointed the token was still there post-1.0, but then saw this merge and wasn't sure what I needed to do in order to get it out.

For anyone else:
git config get ... is insufficient to trigger it, but any set will do it, even if it's a noop, e.g. git config set git_protocol "$(git config get git_protocol)".

mislav added 9 commits June 2, 2020 13:19

Un-publish Hosts from the Config interface

43a8b31

It's not being depended on anywhere right now, so this minimizes the public interface.

Allow stubbing multiple config files

f42c9d4

Allow writing host-specific keys in a blank config

fd7b87f

Trigger OAuth flow only when requesting auth token

77227a6

Previously we would trigger OAuth flow when the config file did not exist. Now we will let an empty Config object be initialized in that case, but trigger OAuth flow when the Context caller requests an AuthToken.

Handle HTTP errors in HasScopes

1595d3b

🔥 hosts optimization

d6f58fb

We dynamically add hosts on `Set`, so this `hosts` cache might fall out of date. We could ensure to keep it updated, but I'm not convinced it's necessary for speed right now.

Enable reading from and writing to empty config files

bad138e

Write all per-host config entries to hosts.yml

c08d4f0

Read from and write to the `hosts.yml` file every time `config.yml` is accessed. Everything that before went under the `hosts:` map now belongs to `hosts.yml`.

🔥 unused constant

8e70fe9

mislav requested review from probablycorey and vilmibm June 2, 2020 13:52

mislav commented Jun 2, 2020

View reviewed changes

vilmibm approved these changes Jun 2, 2020

View reviewed changes

probablycorey reviewed Jun 2, 2020

View reviewed changes

probablycorey approved these changes Jun 2, 2020

View reviewed changes

mislav added 4 commits June 3, 2020 13:43

Un-export AuthFlow

c1a518e

Fix overriding OAuth client ID & secret at build time

bee1323

Rename fn to filename across the config package

5b872e7

Update headless authentication instructions

b48237a

mislav added 2 commits June 4, 2020 12:00

Merge remote-tracking branch 'origin' into auth-split

1f61685

Ensure consistent order of yaml keys in migrateConfig

9a5b628

mislav merged commit 1036666 into trunk Jun 4, 2020

mislav deleted the auth-split branch June 4, 2020 10:49

This was referenced Jul 1, 2020

Support hosts.yml existing while config.yml does not #1304

Merged

~/.config/gh/hosts.yml stopped working to authenticate #1320

Closed

cagiti mentioned this pull request Nov 12, 2020

fix: supporting hosts in config.yml and in hosts.yml for gh cli plumming/dx#73

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write per-host config info to `hosts.yml` instead of `config.yml`#1077

Write per-host config info to `hosts.yml` instead of `config.yml`#1077
mislav merged 15 commits intotrunkfrom
auth-split

mislav commented Jun 2, 2020

Uh oh!

mislav Jun 2, 2020

Uh oh!

mislav Jun 2, 2020

Uh oh!

mislav Jun 2, 2020

Uh oh!

vilmibm left a comment

Uh oh!

vilmibm Jun 2, 2020

Uh oh!

mislav Jun 3, 2020

Uh oh!

probablycorey left a comment

Uh oh!

probablycorey Jun 2, 2020

Uh oh!

mislav commented Jun 3, 2020

Uh oh!

vilmibm commented Jun 3, 2020

Uh oh!

OJFord commented Sep 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		var hosts map[string][]map[string]string
		err = yaml.Unmarshal(b, &hosts)

Conversation

mislav commented Jun 2, 2020

Uh oh!

mislav Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

mislav Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

mislav Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

vilmibm left a comment

Choose a reason for hiding this comment

Uh oh!

vilmibm Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

mislav Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

probablycorey left a comment

Choose a reason for hiding this comment

Uh oh!

probablycorey Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

mislav commented Jun 3, 2020

Uh oh!

vilmibm commented Jun 3, 2020

Uh oh!

OJFord commented Sep 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants