Skip to content

Added parsing of pluggable monitors in package_index.json #1449

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 2 commits into from
Sep 15, 2021

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Sep 13, 2021

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • The PR follows our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?
    Added functions for parsing pluggable monitor fields in package_index.json.
    The added functions are sort of duplicated from the ones that handle pluggable discoveries, I have another PR to factor out the common part but is a bit more involved, so for now I'll push this one so we can move forward.

@cmaglie cmaglie changed the title Pluggable monitor index Added parsing of pluggable monitors in package_index.json Sep 13, 2021
Comment on lines +131 to +155
func (packages Packages) GetPlatformReleaseMonitorDependencies(release *PlatformRelease) ([]*ToolRelease, error) {
if release == nil {
return nil, fmt.Errorf(tr("release cannot be nil"))
}

res := []*ToolRelease{}
for _, monitor := range release.MonitorDependencies {
pkg, exists := packages[monitor.Packager]
if !exists {
return nil, fmt.Errorf(tr("package %s not found"), monitor.Packager)
}
tool, exists := pkg.Tools[monitor.Name]
if !exists {
return nil, fmt.Errorf(tr("tool %s not found"), monitor.Name)
}

// We always want to use the latest available release for monitors
latestRelease := tool.LatestRelease()
if latestRelease == nil {
return nil, fmt.Errorf(tr("can't find latest release of %s"), monitor.Name)
}
res = append(res, latestRelease)
}
return res, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is identical to GetPlatformReleaseDiscoveryDependencies apart from line 137, we could make generic.

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, also MonitorDependencies is identical to DiscoveryDependencies.

The problem is that release.MonitorDependencies is an array of MonitorDependecy... we can make it generic if we change the type of release.DiscoveryDependencies and release.MonitorDependencies to be the same type (for example reusingToolDependencies for both) but this change starts a chain reaction... I've another branch ready for that but I prefer to push it in another PR for review and not delay this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right right, the type is different. Let's leave as it is now for the time being.

Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

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

Small change to make, rest is good.

@cmaglie cmaglie force-pushed the pluggable-monitor-index branch from 7d2579a to 4ea98d6 Compare September 14, 2021 16:09
@cmaglie cmaglie force-pushed the pluggable-monitor-index branch from 4ea98d6 to 9c1d3ca Compare September 14, 2021 16:14
@cmaglie cmaglie force-pushed the pluggable-monitor-index branch from 9c1d3ca to cce97bb Compare September 15, 2021 10:11
@cmaglie cmaglie merged commit 9f160e8 into arduino:master Sep 15, 2021
@cmaglie cmaglie deleted the pluggable-monitor-index branch September 15, 2021 10:59
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