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

Users upgrading gh to v2.40.0 may see failures to write the config file #4744

Closed
williammartin opened this issue Dec 7, 2023 · 19 comments · Fixed by #4749
Closed

Users upgrading gh to v2.40.0 may see failures to write the config file #4744

williammartin opened this issue Dec 7, 2023 · 19 comments · Fixed by #4749

Comments

@williammartin
Copy link

Description

In gh v2.40.0, we are going to be releasing initial support for multiple accounts on a single host. As part of this process there is a migration of on-disk data and we will be adding a version field with value 1 to the config.yml file.

I'm not too familiar with nix or how people use it, so I'm not sure if the following is even an expected workflow. If it is expected that you can update the version of gh in place without regenerating the config.yml file from home-manager scripts it is likely that users will run into an error when gh tries to write the config.yml file.

Although it's our position that gh manages its own configuration file, we also try not to break users that choose to configure their systems this way. Unfortunately, in this case we value the future maintainability of having this version to support future migrations should we need them.

As such, this is just an informative issue for people to find that this new field will be required to perform any operations with v2.40.0. Since I'm not totally sure on the workflows used in this project, here are some possible workarounds:

  1. Temporarily allow writing to config.yml one time so that this version can be written
  2. After seeing a failure, install updated home-manager scripts with version: 1 declared in the config
  3. Update the home-manager scripts with version: 1 declared in the config and then blow everything away and start with v2.40.0 from scratch

To me it feels like option 3 is the most sensible, since home manager enforces config to be declaratively defined up front, the only consequence will be requiring gh auth login to be run for any accounts that were removed when blowing it all away. However, I will again point to my ignorance in this space 😅

We apologise for the inconvenience.

@msfjarvis
Copy link
Contributor

From what I can tell, older versions of gh do not complain if the version key needed by v2.40.0 is present so it should be quite trivial to just always insert that from the home-manager module. I'll raise a PR in a bit.

@williammartin
Copy link
Author

williammartin commented Dec 8, 2023

@msfjarvis ahhh this is possible but might have some unintended consequences. I'm not totally sure whether its intended that your hosts.yml persists across upgrades of gh when using nix and home-manager. In the case that they are well, read on:

The data migration that occurs to support multiple users will only run if the version is not present. So if the version key is in there for an older version of gh, then when the user upgrades to v2.40.0 the migration will not run. The consequence of this is that there will be some strange behaviour for users on v2.40.0 where data is expected to be in the new schema. See below where I mimic what you are describing:

Using 2.39.2:

➜  cli git:(v2.39.2) gh version
gh version 2.39.2 (2023-12-08)
https://github.com/cli/cli/releases/tag/v2.39.2

Log in to an account williammartin:

➜  cli git:(v2.39.2) gh auth login
? What account do you want to log into? GitHub.com
? What is your preferred protocol for Git operations? HTTPS
? How would you like to authenticate GitHub CLI? Login with a web browser

! First copy your one-time code: EC5A-8DC9
Press Enter to open github.com in your browser...
✓ Authentication complete.
- gh config set -h github.com git_protocol https
✓ Configured git protocol
✓ Logged in as williammartin

➜  cli git:(v2.39.2) gh auth status
github.com
  ✓ Logged in to github.com as williammartin (keyring)
  ✓ Git operations for github.com configured to use https protocol.
  ✓ Token: gho_************************************
  ✓ Token scopes: gist, read:org, repo, workflow

See that the hosts file is in the old format:

➜  cli git:(v2.39.2) cat ~/.config/gh/hosts.yml
github.com:
    user: williammartin
    git_protocol: https

Write the version to the config file to do what your suggestion would do:

➜  cli git:(v2.39.2) echo 'version: "1"' >> ~/.config/gh/config.yml
➜  cli git:(v2.39.2) cat ~/.config/gh/config.yml
version: "1"

Now log in with v2.40.0 to wilmartin_microsoft:

➜  cli git:(v2.40.0) gh auth login
? What account do you want to log into? GitHub.com
? What is your preferred protocol for Git operations on this host? HTTPS
? How would you like to authenticate GitHub CLI? Login with a web browser

! First copy your one-time code: EAEB-CA77
Press Enter to open github.com in your browser...
✓ Authentication complete.
- gh config set -h github.com git_protocol https
✓ Configured git protocol
✓ Logged in as wilmartin_microsoft

And we can see that the hosts.yml file has lost knowledge of the previous account:

➜  cli git:(v2.40.0) cat ~/.config/gh/hosts.yml
github.com:
    user: wilmartin_microsoft
    git_protocol: https
    users:
        wilmartin_microsoft:

This occurred because login updates the active user under the host but since williammartin was not migrated under the users key, it was forgotten about altogether.

This isn't really the end of the world, it just means that the user would need to log in to the first account again but you know, it could confuse people that expected to have a new account added since they got the new version.

It's probably not confusing than any other option though 😅

@msfjarvis
Copy link
Contributor

Hm yeah that's definitely a problem. I'll give this some more thought and see if a better solution reveals itself to me.

@williammartin
Copy link
Author

Is there any way for home-manager to do side effects? The migration of data is really very simple. Perhaps it could be mimic-ed here?

There's some bad yaml formatting occurring in there by gofmt but the gist should be clear.

@msfjarvis
Copy link
Contributor

Is there any way for home-manager to do side effects? The migration of data is really very simple. Perhaps it could be mimic-ed here?

There's some bad yaml formatting occurring in there by gofmt but the gist should be clear.

There's onChange but it runs after the config file is generated :(

@williammartin
Copy link
Author

williammartin commented Dec 8, 2023

Is that a problem? The hosts.yml file is unmanaged by home-manager (as far as I can tell) so once config.yml gets generated, could we use onChange to migrate hosts.yml. Instead of leaning on the version existing in config.yml, this mimiced migration could lean on whether there is a users key in the hosts.yml or something?

It might be pretty hard to write the migration though without extra tooling that can work with yaml.

@msfjarvis
Copy link
Contributor

Is that a problem? The hosts.yml file is unmanaged by home-manager (as far as I can tell) so once config.yml gets generated, could we use onChange to migrate hosts.yml. Instead of leaning on the version existing in config.yml, this mimiced migration could lean on whether there is a users key in the hosts.yml or something?

Hm yeah that's true, I'll give it a shot.

It might be pretty hard to write the migration though without extra tooling that can work with yaml.

Nixpkgs has the ability to at least generate YAML, I'll have to check if we can also parse it.

@Tenzer
Copy link

Tenzer commented Dec 11, 2023

For people that are coming here because they encounter an error such as:

failed to migrate configuration: failed to write config after migration: open /Users//.config/gh/config.yml: permission denied

then a workaround I've found is that I could configure gh like this, as a workaround for now:

programs.gh = {
  enable = true;

  settings = {
    # Workaround for https://github.com/nix-community/home-manager/issues/4744
    version = 1;

    ...
  };
};

The migration of hosts.yml has already happened at this point so there's no more manual work to be done.

@williammartin
Copy link
Author

@Tenzer thank you for chiming in. That will indeed work and I don't think it will give you any more issues moving forwards.

@msfjarvis I thought a bit more about writing a migration and I think maybe it's not the best idea because we might end up diverged at some point. It just seems better to hash out a solution that works generally going forward. I'll speak with the team about it today as well.

@msfjarvis
Copy link
Contributor

@msfjarvis I thought a bit more about writing a migration and I think maybe it's not the best idea because we might end up diverged at some point. It just seems better to hash out a solution that works generally going forward. I'll speak with the team about it today as well.

I agree. I'm currently out on vacation so it's been harder to find time to iterate on this, but a potential solution I thought of is to introduce a beforeChange file hook to home-manager and run something like gh auth status there to force the migration and only alter the config.yml after. With stdout and stderr silenced it'd be transparent to users. Unfortunately I can only work on an implementation of that around Friday of this week when I get back home.

@Janik-Haag
Copy link
Member

Hi, thanks for the heads up, I looked into it and don't really see too many options.
The least bad solution on our end would probably running the gh migration in the home-manager activation script, this would require the gh migration to be callable in a shell script, be idempotent and take less than a few seconds because it would be run every time home-manager is activated.

Then we can add the correct version string based on cfg.package.version and the config.stateVersion in the home-manger gh module. If anyone want's to implement that, take a look at: lib.versions. and this "example". As you can see when running nix eval nixpkgs#legacyPackages.x86_64-linux.gh.version it will return the version string: "2.40.0"

@rycee
Copy link
Member

rycee commented Dec 23, 2023

In this case the ideal would be to make the module keep using the old version of gh for stateVersion ≤ 23.11 and the newer version for later state versions. This is the exact use-case for state versions. If the user would like to upgrade, then they have to do some manual actions to migrate to the newer gh and then switch their state version.

That said, it seems like the old version of gh does not exist in Nixpkgs so I guess it needs some alternative solution.

To this end. Is it possible to detect whether the migration has happened using grep, yq, or similar? If so, then you could add a check step in the activation script. Something like:

home.activation.checkGhV1Migration = hm.dag.entryBefore ["writeBoundary"] ''
  if grep -q "magic stuff" "$HOME/whatever"; then
    _iError $'The existing gh state files are not compatible with gh version 2.40.0.
To migrate the state please run the command suitable for your installation method:

  Channel: nix-shell <nixpkgs> -p …
  Flake:   nix run …'
    exit 1
  fi
'';

It is a bit drastic, perhaps, but at least it'll be clear for the user what is going on.

@water-sucks
Copy link

water-sucks commented Dec 24, 2023

Is the version key the only thing that matters? Even if the migration is done manually by specifying the key in the config, I think there are still problems with the multiple accounts feature itself. I tried switching accounts and this failed due to a read-only file system when config.yml was not writable due to being managed by HM, which doesn't make sense to me because it looks like gh doesn't modify the config.yml file when switching anyway.

X Failed to switch account for github.com to water-sucks
open /home/<username>/.config/gh/config.yml: read-only file system

Edit: actually, the error message shows up, but the account itself is switched after the operation. I still think it's a weird problem that this error shows up in the first place when the operation completes successfully though.

@williammartin
Copy link
Author

To this end. Is it possible to detect whether the migration has happened using grep, yq, or similar

Yes, in that writing the version key is the last thing that happens.

but the account itself is switched after the operation. I still think it's a weird problem that this error shows up in the first place when the operation completes successfully though.

Switching account should not result in writing the config.yml and I would consider this to be a bug in gh. That would not be acceptable behaviour in my mind for the level of support we want to provide to this community. I can't quite put the mental model together for why this would be happening but I will investigate after the holidays. Please consider creating an issue over on https://github.com/cli/cli so we can help in a structured manner.


All this said, I have to be honest that I did not expect this to be so problematic for the home-manager community and that feels bad. I have been mulling over another option here which would remove the need to write the config.yml file, that we could ship in January and then skip this version altogether. I won't commit to it now because we're in the holidays and I want to speak to the team, but I did want to write something here to be transparent in case people want to delay work to see how that develops.

I've also mentioned over on the gh issue for this that there's nothing specific about these kind of immutable configurations that we don't want to support, however, I don't want gh to go about it unintentionally and be restricted in our ability to deliver valuable experiences (and, as you can tell from my naivety in this thread already, I don't feel equipped yet to support it intentionally). What do you think the best way to engage with this community is, to understand the goals, the size of the user base, what people here are using gh for, tools in the same space that might be supported in the same way, etc?

@rycee
Copy link
Member

rycee commented Dec 24, 2023

Looking through the comments again it seems like it wouldn't be too tricky to do the migration as part of the activation script (as suggested by @Janik-Haag. Something like

home.activation.migrateGhAccounts =
  hm.dag.entryBetween [ "linkGeneration" ] [ "writeBoundary" ] ''
    # Check if migrated and, if not, run some command that does the migration.
  '';

That should make the migration run after the activation script is allowed to mutate the user environment but before the updated settings (which would include version: 1) are linked into place. If somebody could come up with some shell code to replace the comment, then please give it a try and, if it seems to work OK, open a PR. I promise to have a look and merge ASAP.

Since Nixpkgs unstable only has 2.40.0+ available I think we can do this migration unconditionally and not worry about looking at the package .version field, but if you feel like doing it I don't mind.

@williammartin
Copy link
Author

williammartin commented Dec 24, 2023

If somebody could come up with some shell code to replace the comment, then please give it a try and, if it seems to work OK, open a PR. I promise to have a look and merge ASAP.

Any command will attempt to run the migration, so gh help is probably the simplest thing that could be done.

I would expect something like this to work, assuming that grep is available and it is POSIX compliant:

grep --quiet 'version: "1"' <CONFIG_FILE_LOCATION> || gh help

This will search for the string version: "1" in the config file, but silencing the output. If found, then the left side evaluates to true so the right side won't be run, and if not found, then the right side would be run ensuring the migration occurs.

I'm not sure all the environments in which home-manager might be used but I think the above is pretty well standardised?

That said, the first thing that gh when migrating is check whether that version exists, so it would be just a minor saving of exec-ing the process compared to just running gh help.

I can't speak to the package .version stuff, so I'll leave that to someone else.

@rycee
Copy link
Member

rycee commented Dec 25, 2023

Ok, cool. I've updated the #4749 PR to include this activation script thingy. It will run gh help if there already exists a gh configuration file. Untested, though. It would be great if somebody could try it out and see if it works OK!

@water-sucks
Copy link

Switching account should not result in writing the config.yml and I would consider this to be a bug in gh. That would not be acceptable behaviour in my mind for the level of support we want to provide to this community. I can't quite put the mental model together for why this would be happening but I will investigate after the holidays. Please consider creating an issue over on https://github.com/cli/cli so we can help in a structured manner.

I've created an issue there. Thanks!

trueNAHO added a commit to trueNAHO/dotfiles that referenced this issue Dec 27, 2023
Update all inputs, and temporarily resolve 'gh' migration issues by
replacing the 'github:nix-community/home-manager' branch with
'github:msfjarvis/home-manager/hs/gh-2.40-compat'.

Additionally, remove the now redundant 'beautysh' input.

Related: nix-community/home-manager#4744
Requires: nix-community/home-manager#4749
@williammartin
Copy link
Author

Amazing, thank you @msfjarvis and @rycee for the fix and everyone else for their involvement in this issue.

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 a pull request may close this issue.

6 participants