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 resource discovery code #422

Merged
merged 15 commits into from
Apr 27, 2024

Conversation

SteveL-MSFT
Copy link
Member

PR Summary

In preparation to add support for handle multiple and finding specific versions of resource, needed to refactor the existing code.

  • New helper to simply find all command based resources
  • Another helper to enumerate adapter resources
  • All the discovery is cached in memory
  • Rename discover_resources() to be find_resources() as that's really what it does based on a filter
  • Functions now return a vector of resources of the same type name, but different versions

Because the changes moves around lots of existing code, might be easier to review the file rather than the diff.

@SteveL-MSFT SteveL-MSFT marked this pull request as draft April 24, 2024 22:41
@SteveL-MSFT SteveL-MSFT marked this pull request as ready for review April 26, 2024 00:41
resources.push(resource.1);
for (_resource_name, found_resources) in discovered_resources {
for resource in found_resources {
resources.push(resource.clone());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this PR adds multi-version support only for 'List' operation, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, that's just in preparation for follow-up PR, didn't want to change find_resource yet

@@ -5,6 +5,8 @@ use crate::{dscresources::dscresource::DscResource, dscerror::DscError};
use std::collections::BTreeMap;

pub trait ResourceDiscovery {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this refactoring PR is a good time to get rid of trait ResourceDiscovery, since we only have 1 type of resources.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should do that separately and also remove the abstraction above command_resource since we only have that one

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #426

use tracing_indicatif::span_ext::IndicatifSpanExt;

pub struct CommandDiscovery {
// use BTreeMap so that the results are sorted
Copy link
Collaborator

Choose a reason for hiding this comment

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

please extend the comment to something like :
... so that results are sorted by reousrce type name and vector values - are found resource versions of this type in order of decreasing of semversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@SteveL-MSFT SteveL-MSFT added this pull request to the merge queue Apr 27, 2024
Merged via the queue into PowerShell:main with commit 0157276 Apr 27, 2024
4 checks passed
@SteveL-MSFT SteveL-MSFT deleted the refactor-discover branch April 27, 2024 01:17
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.

None yet

2 participants