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

Store configuration status and enable remote watch events #4647

Merged
merged 15 commits into from
Jul 29, 2024

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Jul 17, 2024

Change

This change stores ongoing status information during configuration application in the database. This data is then retrievable via the existing API surface from a set retrieved from GetConfigurationHistory. The data includes: configuration set state, configuration set apply start/stop times, configuration unit state, and configuration unit results.

In addition, the existing events on the ConfigurationProcessor and ConfigurationSet are implemented to send data when appropriate. This works across processes, using named events to indicate changes and the database to carry the actual data.

winget configure list is updated to expose this information, for example:

PS > wingetdev configure list
Identifier                             Name      State     Origin
--------------------------------------------------------------------
{7D5CF50E-F3C6-4333-BFE6-5A806F9EBA4E} Test Name Completed Test Path
{51837FDE-C290-408B-A847-B176DB184B89} Test Name Completed Test Path
{E7367B3D-07A2-4875-A190-C369B79E6F5B} Test Name Completed Test Path
{B186F2D0-3245-4AD1-A712-260906033E57} Test Name Completed Test Path
{2AA76541-A0DF-45EE-91F4-401F93FA88CD} Test Name Completed Test Path
PS > wingetdev configure list --history 7D5CF50E
Field         Value
----------------------------------------------------
Identifier    {7D5CF50E-F3C6-4333-BFE6-5A806F9EBA4E}
Name          Test Name
Origin        Test Origin
Path          Test Path
State         Completed
First Applied 2024-07-16 21:15:13.000
Apply Begun   2024-07-16 21:15:13.000
Apply Ended   2024-07-16 21:15:13.000

Unit                         State     Result
-------------------------------------------------
Module/Resource [Name]       Completed 0x00000000
Module/Resource2 [Name2]     Completed 0x00000000
Module2/Resource [Group]     Completed 0x00000000
|-Module3/Resource [Child1]  Completed 0x00000000
|-Module4/Resource2 [Child2] Completed 0x00000000

 

Validation

Updated tests to check for status data. Manually verified eventing.

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner July 17, 2024 04:33

This comment has been minimized.

This comment has been minimized.

…er tests; remove pending state from progress if present to prevent inconsistencies; add verbose to spurious test failure
florelis
florelis previously approved these changes Jul 26, 2024
Copy link
Member

@florelis florelis left a comment

Choose a reason for hiding this comment

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

lgtm, but I'll admit that a lot of the nuance was lost on me...

@@ -58,6 +59,10 @@ namespace AppInstaller::CLI
context << ShowSingleConfigurationSetHistory;
}
}
else if (context.Args.Contains(Execution::Args::Type::ConfigurationStatusWatch))
Copy link
Member

Choose a reason for hiding this comment

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

Should we validate that this argument is not used with ConfigurationHistoryItem?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is more of a diagnostic feature, which is why I hid the argument and it uses some unlocalized strings (although they are the symbolic names of enums, so I feel less bad). If one does specify it with the history arg, it will just be ignored (for now, it could actually do the same thing but for the one set).

Comment on lines +2025 to +2028
if (currentSiblings.Depth)
{
unitStream << '|' << std::string((currentSiblings.Depth * 2) - 1, '-');
}
Copy link
Member

Choose a reason for hiding this comment

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

So the format is like

a
b
|-b1
|---b1.1
|---b1.2
|-b2

I expected more of a tree structure rather than everything rooted at the top level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the goal was that a flat list would not need any additional characters, while child items would hopefully be clearly indicated.

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp Outdated Show resolved Hide resolved
Comment on lines 300 to 301
OutputColumns(m_stream, "MAX(", column);
m_stream << ")";
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The way the parentheses are split in these two statements feels weird... Since QualifiedColumn has an operator<<, this could also be m_stream << "MAX(" << column << ")";. There is also a version of OutputColumn that takes an Aggregate and handles the MAX already

Copy link
Member Author

Choose a reason for hiding this comment

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

So this is why I thought it was weird there wasn't already a method... there was.

Copy link
Member

Choose a reason for hiding this comment

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

Most of these methods are almost identical. Can we do anything to reduce the duplication?

Copy link
Member Author

Choose a reason for hiding this comment

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

Used some template methods to remove ~250 lines of repetitive code.

@JohnMcPMS JohnMcPMS merged commit 2e91480 into microsoft:master Jul 29, 2024
8 checks passed
@JohnMcPMS JohnMcPMS deleted the config-status branch July 29, 2024 17:46
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