Skip to content

Adding safety timeouts for package manager operations and increasing the test limit for remediations #946

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

Merged
merged 7 commits into from
Apr 1, 2025

Conversation

MariusNi
Copy link
Contributor

@MariusNi MariusNi commented Mar 28, 2025

Description

Adding safety timeouts for package manager operations (30 minutes, which can act during RC/DC where we do not have overall timeouts like Azure Policy) and increasing the test limit for remediations (5 minutes per rule, this being the actual fix to the random CI failures, problem being that we just had too strict limits there, which we expected and now thanks to such findings we are adjusting).

Checklist

  • I have read the contribution guidelines.
  • All unit tests are passing.
  • I have merged the latest dev branch prior to this PR submission.
  • I ran pre-commit on my changes prior to this PR submission.
  • I submitted this PR against the dev branch.

@MariusNi MariusNi requested a review from a team as a code owner March 28, 2025 21:01
Copy link

github-actions bot commented Mar 28, 2025

Test Results

 44 files  ±0   44 suites  ±0   37m 19s ⏱️ -9s
  4 tests ±0    4 ✅ ±0   0 💤 ±0  0 ❌ ±0 
176 runs  ±0  154 ✅ ±0  22 💤 ±0  0 ❌ ±0 

Results for commit 6feb1ee. ± Comparison against base commit d050c32.

♻️ This comment has been updated with latest results.

simonjaeger
simonjaeger previously approved these changes Mar 28, 2025
AhmedBM
AhmedBM previously approved these changes Mar 28, 2025
@wupeka
Copy link
Contributor

wupeka commented Mar 31, 2025

Could this PR be split into two? The changes in ExecuteCommand seem to be completely unrelated to the core ones.

Copy link
Contributor

@robertwoj-microsoft robertwoj-microsoft left a comment

Choose a reason for hiding this comment

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

If possible, please split this PR into separate PRs:

  • timeout changes
  • style changes

@MariusNi
Copy link
Contributor Author

If possible, please split this PR into separate PRs:

  • timeout changes
  • style changes

Will do.

@MariusNi
Copy link
Contributor Author

MariusNi commented Mar 31, 2025

Could this PR be split into two? The changes in ExecuteCommand seem to be completely unrelated to the core ones.

Done. Removed all changes from CommandUtils.c to issue a separate PR for those.

They were related because as described in the original PR description, the timeout added in PackageUtils.c depends on this reworked code here. There are 2 lines with TAB characters and various other minor code formatting issues in CommandUtils.c which can create a bad precedent for this code (curious how the TABs escaped with the pre-commit rule in place, when I run pre-commit locally they are caught). Will open a separate PR for these cosmetic issues as you asked.

@MariusNi MariusNi closed this Mar 31, 2025
@MariusNi MariusNi reopened this Mar 31, 2025
@MariusNi MariusNi force-pushed the MariusNi/OsConfig_TimeoutPM_Mar28_2025 branch from 6bf3feb to f836f4b Compare March 31, 2025 17:35
@MariusNi MariusNi merged commit c4f29ca into dev Apr 1, 2025
109 checks passed
@MariusNi MariusNi deleted the MariusNi/OsConfig_TimeoutPM_Mar28_2025 branch April 1, 2025 20:38
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.

6 participants