Skip to content

Clarify SelectedPackageTypes meaning and improve its usage #30142

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.

Sign up for GitHub

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 1 commit into from
Feb 16, 2022
Merged

Clarify SelectedPackageTypes meaning and improve its usage #30142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions dev-tools/mage/crossbuild.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ const defaultCrossBuildTarget = "golangCrossBuild"
// See NewPlatformList for details about platform filtering expressions.
var Platforms = BuildPlatforms.Defaults()

// SelectedPackageTypes is the list of package types
// SelectedPackageTypes is the list of package types. If empty, all packages types
// are considered to be selected (see isPackageTypeSelected).
var SelectedPackageTypes []PackageType

func init() {
Expand All @@ -65,7 +66,7 @@ func init() {
}
}

// CrossBuildOption defines a option to the CrossBuild target.
// CrossBuildOption defines an option to the CrossBuild target.
type CrossBuildOption func(params *crossBuildParams)

// ImageSelectorFunc returns the name of the builder image.
Expand Down
18 changes: 10 additions & 8 deletions dev-tools/mage/pkg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,19 @@ func Package() error {
return nil
}

// isPackageTypeSelected returns true if SelectedPackageTypes is empty or if
// pkgType is present on SelectedPackageTypes. It returns false otherwise.
func isPackageTypeSelected(pkgType PackageType) bool {
if SelectedPackageTypes != nil {
selected := false
for _, t := range SelectedPackageTypes {
if t == pkgType {
selected = true
}
if len(SelectedPackageTypes) == 0 {
return true
}

for _, t := range SelectedPackageTypes {
if t == pkgType {
return true
Copy link
Contributor

@ph ph Feb 1, 2022

Choose a reason for hiding this comment

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

Not exactly related to this change.
But the name of the function isPackageTypeSelected and the behavior of this method looks reversed to me.
It returns true when SelectedPackageTypes is nil or slice is empty?

Copy link
Member Author

@AndersonQ AndersonQ Feb 2, 2022

Choose a reason for hiding this comment

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

yes, it does. However from what I could understand, an empty/nil SelectedPackageTypes means, the environment variable PACKAGES isn't defined, therefore all package types are selected. Thus the function returning true when SelectedPackageTypes is either nil or empty.

Copy link
Member Author

@AndersonQ AndersonQ Feb 2, 2022

Choose a reason for hiding this comment

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

That's is counter intuitive, so I added the docs here and on the SelectedPackageTypes as well.

Copy link
Contributor

@ph ph Feb 3, 2022

Choose a reason for hiding this comment

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

Yes, agree, my brain did not compute correctly the func name, thanks for adding clarification.

}
return selected
}
return true
return false
}

type packageBuilder struct {
Expand Down