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

refactor: run pre-sudo before pre-cmds && clear cached credential aft… #565

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SteveLauC
Copy link
Member

@SteveLauC SteveLauC commented Oct 5, 2023

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

For new steps

  • Optional: Topgrade skips this step where needed
  • Optional: The --dry-run option works with this step
  • Optional: The --yes option works with this step if it is supported by
    the underlying command

If you developed a feature or a bug fix for someone else and you do not have the
means to test it, please tag this person here.


This PR

  1. Move pre-sudo before all the steps
  2. Clear cached credential after execution

@SteveLauC
Copy link
Member Author

Here is a build with this patch included: https://github.com/SteveLauC/topgrade/releases/tag/pr565

@agoodshort, would you like to give it a test?

@agoodshort
Copy link

Let me find a way to reproduce the issue from #372 and I am happy to test yes!


May I suggest to update these comments too?

topgrade/src/config.rs

Lines 1427 to 1428 in 3626c9c

/// If `true`, `sudo` should be called after `pre_commands` in order to elevate at the
/// start of the session (and not in the middle).

# Run `sudo -v` to cache credentials at the start of the run

@SteveLauC
Copy link
Member Author

May I suggest to update these comments too?

Sure! Done!

Can I trouble you for a code review on this PR :)

@agoodshort
Copy link

Hey @SteveLauC sorry, I am going on holidays tomorrow and didn't have a chance to reproduce the issue. I'm dropping a comment on #372 to see if author would volunteer to test, otherwise I will do it when I'm back. Code looks good, but I don't want to fully review without a proper test.

@SteveLauC
Copy link
Member Author

I am going on holidays tomorrow

Enjoy your holiday! :)

I'm dropping a comment on #372 to see if author would volunteer to test, otherwise I will do it when I'm back. Code looks good, but I don't want to fully review without a proper test.

Sure!

@SteveLauC
Copy link
Member Author

Well, I just realized that pre-sudo should be placed after self_update

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