Skip to content

Conversation

@hardy-ethan
Copy link
Contributor

@hardy-ethan
Copy link
Contributor Author

Closes #201

@hardy-ethan
Copy link
Contributor Author

hardy-ethan commented Jan 13, 2025

I'm not sure if the host key warning needs to be included, or if the way it's included is good enough...

I don't think students have accessed the Linux Remote Login Service before taking 280, but I took 280 my first semester, so I wouldn't have had a chance to see it before 280.

I like having a mention of the warning so as not to cause concern (and to not accidentally ingrain "just say yes to ssh warnings").

Copy link
Contributor

@awdeorio awdeorio left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I found one tiny spot that you might've missed:

$ ag -Q login.engin.umich.edu
docs/setup_caen.md
63:    ~/.ssh/known_hosts:1: login.engin.umich.edu

@hardy-ethan
Copy link
Contributor Author

hardy-ethan commented Jan 13, 2025

Overall LGTM, I found one tiny spot that you might've missed:

$ ag -Q login.engin.umich.edu

docs/setup_caen.md

63:    ~/.ssh/known_hosts:1: login.engin.umich.edu

That's intended, it's part of the warning students may get when using the new URL:

The host public key of the old and new CAEN URLs are the same. The first time someone who has used the old URL used the new one, they will get warned that the CAEN host key is already known as the old URL under the known_hosts file.

Reusing the host key is sometimes an issue, but in the case of this CAEN service, it's fine, so we want the students to accept the connection.

@hardy-ethan hardy-ethan requested a review from awdeorio January 13, 2025 16:41
@awdeorio
Copy link
Contributor

Whoops! That's my mistake for not checking the context when I did the text-search. LGTM

@awdeorio
Copy link
Contributor

awdeorio commented Jan 13, 2025

Because this involves a change to a live semester, I'll tag a current instructor to say whether it's OK to merge now or if we should wait. @jamesjuett @saquibrazak @sofiazxy

@hardy-ethan
Copy link
Contributor Author

hardy-ethan commented Jan 13, 2025

Whoops! That's my mistake for not checking the context when I did the text-search. LGTM

nw, it happens 👍 (it's also easier for me to see because I was the one that wrote it :P)

Copy link
Contributor

@jamesjuett jamesjuett left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @hardy-ethan for making this contribution!

@jamesjuett
Copy link
Contributor

I'm going to go ahead and merge, bypassing branch predictions - the Spec Preview is failing since this a fork and one of the necessary secrets isn't provided in that case.

@jamesjuett jamesjuett merged commit 3e6efbc into eecs280staff:main Jan 16, 2025
1 check failed
@hardy-ethan hardy-ethan mentioned this pull request Jan 16, 2025
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 this pull request may close these issues.

3 participants