-
Notifications
You must be signed in to change notification settings - Fork 147
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
Fix local host detection for remotes with user #755
Conversation
e61b114
to
6c83318
Compare
Codecov ReportAttention: Patch coverage is
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. |
f06aa3c
to
94e8687
Compare
Strip the user when comparing the remote with the hostname of the local machine
Refactor should_execute_remote to make it testable
94e8687
to
6e9cd71
Compare
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
Could you please show me the benefit of this new feature? |
Hi, Sorry for the confusion, the description was missing a word:
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, |
I see two approaches for improving how topgrade handles |
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. |
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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
No need to apologise, I just wasn't sure if I explained the problem clearly enough previously. Thanks for your work on topgrade! |
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 hasuser@host
present in theremote_upgrades
list intopgrade.toml
.Current behavior
topgrade will trigger remote execution for
user@host
over SSH connection tohost
. 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 tohost
and skipped by topgrade.Standards checklist:
CONTRIBUTING.md
cargo build
)cargo fmt
)cargo clippy
)cargo test
)