Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add experimental feature support for enabling Windows Feature dependencies #3005
Add experimental feature support for enabling Windows Feature dependencies #3005
Changes from 30 commits
a6ec947
cea7e0d
8000470
f66dd54
ba74875
6d8ca3e
5d048d4
98b44d7
8ebe76b
d59eb0e
8c8a7cf
d4d9430
a4e0eee
b5b2ae4
61a4143
3ba134a
f23ccb7
6711502
427cc0a
c28f5e4
fa20945
fb1a034
0e027e8
09b1a0f
12c4cf5
e1dd564
fdc86bb
918875f
0f36ef9
13200c3
db3c8ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this already handled by
HasAnyOf
? If not, should it be?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.
Installer dependencies are known before this workflow is ever executed; Specific dependency types are also checked for in the
ReportDependencies
workflow.I'm wondering, now that multiple dependency types will be supported, if it makes more sense to set an execution flag for "Contains Package Dependencies" or "Contains Windows Features Dependencies" in the ReportDependencies flow.
With the flags, then would be possible to have a separate workflow around
CheckSupportForWindowsFeatures
that checks the flag ensures the experimental feature is enabled and user is running as admin, to avoid creating copies of the dependencies data in memory unnecessarilyThere 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 think there's a missing check for a pending reboot if the feature is already enabled?
Thinking of the case where a user attempts an install and gets a failure that a reboot is required. If they attempt to run the install command again without rebooting, the Windows Feature will be shown as enabled(?) and therefore won't trigger the rebootRequired, effectively negating the need to use
--force
by running the command twice.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 changed the condition so that a reboot required is no longer considered as enabled. That means if they run the install command again, the feature would show up as needing a reboot and wouldn't show up as being enabled and a reboot required message would be the shown requiring
--force
to proceed.This comment was marked as resolved.
Sorry, something went wrong.
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.
Changed to report each feature that failed to enable
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.
@denelon - I may be asking the wrong (scope creep) questions here, so forgive me, but would we want any sort of telemetry around these?
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 think we should be showing each feature as its own item and not aggregating them as well.
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.
Changed so that each windows feature is represented as its own item.
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.
Sounds good; I'm presuming any telemetry needs can be added once support is no longer experimental
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 think this neglects the case where one feature fails to enable, and another has a reboot required. I don't know if we care about that case or not, or if that's even a possibility with the way
hr
gets handled.If
--force
isn't present, I don't think it matters, since both the reboot and the fail would cause the context to terminate, and the termination reason doesn't make much of a difference. If--force
is present, the user would only be informed they are bypassing the failed install, and they wouldn't be informed they are also bypassing the reboot requirement.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.
This whole thing doesn't seem like the right way to handle dependencies. At least, "download" is not consistent with "and also, handle getting all dependencies squared away". So at least maybe change the name of this task?
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.
Moved this out of "download" to the step right before
InstallPackageInstaller
.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.
In reality, this is a larger issue than your feature work, and I think it really needs to be addressed before we can enable dependencies on the whole. Neither of us considered that in retrospect.
You should move the workflow task back here (for now); and probably put it before package dependencies. I say that because one of the packages may actually be dependent on the Windows feature (although it really should have declared it). But maybe it is an optional dependency that affects the install time behavior. It is not likely the case that the Windows feature is dependent on any package though.
The issue is that multiple "downloads" are currently allowed via COM, but if dependencies are actually installed as part of "download", then multiple installs are allowed concurrently. So either we need to change the download/install phase split to download everything, then install everything, or we need to support going back and forth between download and install phases as we handle each dependency.
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.
Thanks for the explanation, I didn't know that COM allowed for multiple downloads. I moved the task back to the suggested location and added a TODO comment explaining the issue that needs to be addressed.