Skip to content

Use codespaces.auto instead for the automatic ssh keys#5859

Merged
cmbrose merged 11 commits intotrunkfrom
cmbrose/fix-auto-key-path
Jun 29, 2022
Merged

Use codespaces.auto instead for the automatic ssh keys#5859
cmbrose merged 11 commits intotrunkfrom
cmbrose/fix-auto-key-path

Conversation

@cmbrose
Copy link
Member

@cmbrose cmbrose commented Jun 28, 2022

The new ssh feature to automatically create public/private keys will not create the keys if ~/.ssh/codespaces (the name of the automatically created key) already exists.

At the same time, the --help message and docs for the same command recommend running commands like gh cs ssh --config > ~/.ssh/codespaces to create ssh configs files. If someone has already created that file, the automatic keypair generation won't kick in. Which leads to some confusing messages.

This PR does 2 things:

  1. Changes the name of the generated key to codespaces.auto to avoid the collision
  2. Updates docs to explain that it will create the keys in the first place

@cmbrose
Copy link
Member Author

cmbrose commented Jun 28, 2022

I'm trying to decide on the behavior if there is some weird case where someone also already has a codespaces.auto file. We could just implement a randomly generated name, but that will make it harder to re-use the key later and we might end up generating multiple keys over time. This may also be a non-issue that I'm just trying to solve for though, if anyone has thoughts please chime in 😄

@cmbrose cmbrose marked this pull request as ready for review June 28, 2022 21:41
@cmbrose cmbrose requested a review from a team as a code owner June 28, 2022 21:41
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Jun 28, 2022
@cliAutomation
Copy link
Collaborator

Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message.

Copy link
Contributor

@josebalius josebalius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to write some sort of cleanup logic to delete the old key? If only for a couple of CLI releases? Otherwise, people might think we are polluting the .ssh directory? Just a thought.

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! just some notes about backwards compatibility and clarity of the docs

Copy link

@Kizzer415 Kizzer415 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the migration logic!

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@cmbrose cmbrose enabled auto-merge June 29, 2022 16:48
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re Windows failures: do you think it's possible that the The process cannot access the file because it is being used by another process failures are legitimate; i.e. that the gh process opens a file but forgets to close it?

@cmbrose
Copy link
Member Author

cmbrose commented Jun 29, 2022

@mislav yup, it's a difference in os.Create for windows I guess (or at least Windows is more strict about it) where the file has to be explicitly closed before it an be renamed. So it was just an issue in the test infra, hopefully fixed now

@cmbrose cmbrose merged commit 604adc5 into trunk Jun 29, 2022
@cmbrose cmbrose deleted the cmbrose/fix-auto-key-path branch June 29, 2022 17:35
Copy link

@lokera666 lokera666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeeeeeeeee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants