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

Fix local host detection for remotes with user #755

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

raindev
Copy link
Contributor

@raindev raindev commented Mar 26, 2024

Strip the user when comparing the remote with the hostname of the local machine to prevent repeatedly running topgrade over SSH on localhost.

Details of the issue

Set-up

topgrade is run on a machine with a hostname equal to host, which has user@host present in the remote_upgrades list in topgrade.toml.

Current behavior

topgrade will trigger remote execution for user@host over SSH connection to host. This will repeat indefinitely due to the host recursively connecting to itself during each remote execution.

New behavior

user@host remote will be recognized as being local to host and skipped by topgrade.

Standards checklist:

  • The PR title is descriptive.
  • I have read CONTRIBUTING.md
  • The code compiles (cargo build)
  • The code passes rustfmt (cargo fmt)
  • The code passes clippy (cargo clippy)
  • The code passes tests (cargo test)
  • Optional: I have tested the code myself

@raindev raindev force-pushed the fix-remote-hostname-matching branch from e61b114 to 6c83318 Compare March 26, 2024 21:46
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 90.47619% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 4.05%. Comparing base (728ea26) to head (6e9cd71).

Files Patch % Lines
src/config.rs 92.68% 3 Missing ⚠️
src/main.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #755      +/-   ##
========================================
- Coverage   5.03%   4.05%   -0.99%     
========================================
  Files         37      32       -5     
  Lines      12085    6194    -5891     
========================================
- Hits         609     251     -358     
+ Misses     11476    5943    -5533     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@raindev raindev force-pushed the fix-remote-hostname-matching branch 2 times, most recently from f06aa3c to 94e8687 Compare March 26, 2024 22:28
Strip the user when comparing the remote with the hostname of the local
machine
Refactor should_execute_remote to make it testable
@raindev raindev force-pushed the fix-remote-hostname-matching branch from 94e8687 to 6e9cd71 Compare March 26, 2024 22:32
@raindev raindev marked this pull request as ready for review March 26, 2024 22:33
@SteveLauC
Copy link
Member

Hi, thanks for your interest in contributing to Topgrade!

From what I understand, this seems to be a new feature rather than a bug-fix as it is explicitly documented that the remote host list should only contain hostnames

to repeatedly running topgrade over SSH on localhost.

Could you please show me the benefit of this new feature?

@raindev
Copy link
Contributor Author

raindev commented Mar 31, 2024

Hi,

Sorry for the confusion, the description was missing a word:

to prevent repeatedly running topgrade over SSH on localhost

The ability to specify a user is handy in situations when it differs between the local and the remote machine, both having the a copy of the same topgrade config.

Despite the example config file only describes specifying hostnames of remotes, user@host form already works (except that host will attempt to connect to itself when the remote is present in its own configuration, which the PR resolves). Given remote execution is over SSH, this would likely be what the users expect (an example from a random config found on GitHub) - other tools that leverage SSH would usually also allow to specify a user.

@raindev
Copy link
Contributor Author

raindev commented Apr 8, 2024

I see two approaches for improving how topgrade handles user@ included into a remote: 1) detect this and let the user know that this is not supported 2) take user into account when determining if topgrade should be run remotely. The option 2 is what the pull request implements. Right now including users into a remote will work as expected in some cases but not in the others (see the screenshot below). @SteveLauC let me know what do you think.

topgrade-remote-user

@SteveLauC
Copy link
Member

Hi, I didn't reply, it is not because I am opposed to this change, it is simply because I am busy with my work and couldn't have time to handle these OSS things, I am sorry about this.

@SteveLauC
Copy link
Member

SteveLauC commented Apr 8, 2024

Ok, I just gave this PR a closer look, and the issue described in the PR description indeed exists(thanks for it), the approach proposed in this PR looks good to me!

pub fn should_execute_remote(&self, remote: &str) -> bool {
if let Ok(hostname) = hostname() {
if remote == hostname {
pub fn should_execute_remote(&self, hostname: Result<String>, remote: &str) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

I am curious why we added this new argument (hostname), looks like the previous way how we handled it still works

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I get it, it is for tests

@SteveLauC SteveLauC merged commit 04bfb45 into topgrade-rs:main Apr 8, 2024
14 of 19 checks passed
@raindev
Copy link
Contributor Author

raindev commented Apr 8, 2024

No need to apologise, I just wasn't sure if I explained the problem clearly enough previously.

Thanks for your work on topgrade!

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.

2 participants