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

Runspace Change to fix Tweaks #2719

Closed
wants to merge 1 commit into from
Closed

Conversation

ChrisTitusTech
Copy link
Owner

Type of Change

  • New feature
  • Bug fix
  • Documentation update
  • Refactoring
  • Hotfix
  • Security patch
  • UI/UX improvement

Description

Big changes to the Invoke-Runspace function that will change the way things are sent to the runspace and disposed of.

Testing

Tested multiple tweaks and program installs.

Impact

This will have a LARGE impact on the program as it changes how the runspace works.

Issue related to PR

Resolves issues with PR #2596 and tweaks not applying

Additional Information

@Marterich @MyDrift-user take a look at this refactor and make sure this isn't going to make things go boom.

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no errors/warnings/merge conflicts.

@MyDrift-user
Copy link
Contributor

MyDrift-user commented Sep 11, 2024

My tests gave following results:

  • Install task without Choco preference
  • Install task with Choco preference
  • Tweaks
  • Features

EDIT: Code looks fine to me and it works, but still would wait to look at @Marterich's fix that he announced, then compare and decide from there.

@Marterich
Copy link
Contributor

Marterich commented Sep 11, 2024

My Proposed solutions #2720 and #2721 are also done. I think the truth probably lies somewhere in between all of them. I think the refactor/changes to the runspaces you did look quite fine, but you should probably implement the Choco install fix from #2720 as well.
(Also I'm quite sure that you fixed the issue by accident when you added the second argument $DNSChange because the issue at heard was simply Powershell that doesn't interprets things as a list when there is only one entry (see my second PR))

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.

Latest update won't apply tweaks
3 participants