Merge pull request #195 from github/johnbreault-fix-os-filter-matching

Improve UX around --os-include/--os-exclude and --destination-url

Author: JohnBreault

README.md

@@ -19,7 +19,7 @@ If your GitHub Enterprise Server instance is on a completely isolated network wh
 From a machine with access to both GitHub.com and GitHub Enterprise Server use the `./codeql-action-sync sync` command to copy the CodeQL Action and bundles.
 
 **Required Arguments:**
-* `--destination-url` - The URL of the GitHub Enterprise Server instance to push the Action to.
+* `--destination-url` - The URL of the GitHub Enterprise Server instance to push the Action to. If no protocol scheme is included (e.g. you pass `github.example.com` instead of `https://github.example.com`), `https://` is assumed.
 * `--destination-token` - A [Personal Access Token](https://docs.github.com/en/enterprise/user/github/authenticating-to-github/creating-a-personal-access-token) for the destination GitHub Enterprise Server instance. If the destination repository is in an organization that does not yet exist or that you are not an owner of, your token will need to have the `site_admin` scope in order to create the organization or update the repository in it. The organization can also be created manually or an existing organization that you own can be used, in which case the `repo` and `workflow` scopes are sufficient. The token can also be provided by setting the `CODEQL_ACTION_SYNC_TOOL_DESTINATION_TOKEN` environment variable.
 
 **Optional Arguments:**
@@ -29,8 +29,8 @@ From a machine with access to both GitHub.com and GitHub Enterprise Server use t
 * `--actions-admin-user` - The name of the Actions admin user, which will be used if you are updating the bundled CodeQL Action. If not specified `actions-admin` will be used.
 * `--force` - By default the tool will not overwrite existing repositories. Providing this flag will allow it to.
 * `--push-ssh` - Push Git contents over SSH rather than HTTPS. To use this option you must have SSH access to your GitHub Enterprise instance configured.
-* `--os-include` - A comma-separated list of operating systems (e.g. `linux64,win64`) to include CodeQL bundle release assets for. Cannot be used together with `--os-exclude`. If neither is specified, assets for all operating systems are synced.
-* `--os-exclude` - A comma-separated list of operating systems (e.g. `win64,osx64`) to exclude CodeQL bundle release assets for. Cannot be used together with `--os-include`.
+* `--os-include` - A comma-separated list of operating systems to include CodeQL bundle release assets for. Cannot be used together with `--os-exclude`. If neither is specified, assets for all operating systems are synced. Values must exactly match the OS identifier embedded in the asset name (for example `linux64`, `linux-arm64`, `osx64`, `win64` — not shorthand like `linux` or `win`). If a value does not match any asset in the releases being pulled, a warning listing the OS identifiers that were actually found is logged.
+* `--os-exclude` - A comma-separated list of operating systems to exclude CodeQL bundle release assets for. Cannot be used together with `--os-include`. Values must exactly match the OS identifier embedded in the asset name (for example `linux64`, `linux-arm64`, `osx64`, `win64`).
 * `--compression-format` - The compression format of CodeQL bundle release assets to sync, either `gz` or `zst`. If not specified, both compression formats are synced.
 
 ### I don't have a machine that can access both GitHub.com and GitHub Enterprise Server.
@@ -39,16 +39,16 @@ From a machine with access to GitHub.com use the `./codeql-action-sync pull` com
 **Optional Arguments:**
 * `--cache-dir` - The directory in which to store data downloaded from GitHub.com. If not specified a directory next to the sync tool will be used.
 * `--source-token` - A token to access the API of GitHub.com. This is normally not required, but can be provided if you have issues with API rate limiting. The token does not need to have any scopes.
-* `--os-include` - A comma-separated list of operating systems (e.g. `linux64,win64`) to include CodeQL bundle release assets for. Cannot be used together with `--os-exclude`. If neither is specified, assets for all operating systems are synced.
-* `--os-exclude` - A comma-separated list of operating systems (e.g. `win64,osx64`) to exclude CodeQL bundle release assets for. Cannot be used together with `--os-include`.
+* `--os-include` - A comma-separated list of operating systems to include CodeQL bundle release assets for. Cannot be used together with `--os-exclude`. If neither is specified, assets for all operating systems are synced. Values must exactly match the OS identifier embedded in the asset name (for example `linux64`, `linux-arm64`, `osx64`, `win64` — not shorthand like `linux` or `win`). If a value does not match any asset in the releases being pulled, a warning listing the OS identifiers that were actually found is logged.
+* `--os-exclude` - A comma-separated list of operating systems to exclude CodeQL bundle release assets for. Cannot be used together with `--os-include`. Values must exactly match the OS identifier embedded in the asset name (for example `linux64`, `linux-arm64`, `osx64`, `win64`).
 * `--compression-format` - The compression format of CodeQL bundle release assets to sync, either `gz` or `zst`. If not specified, both compression formats are synced.
 
 Next copy the sync tool and cache directory to another machine which has access to GitHub Enterprise Server.
 
 Now use the `./codeql-action-sync push` command to upload the CodeQL Action and bundles to GitHub Enterprise Server.
 
 **Required Arguments:**
-* `--destination-url` - The URL of the GitHub Enterprise Server instance to push the Action to.
+* `--destination-url` - The URL of the GitHub Enterprise Server instance to push the Action to. If no protocol scheme is included (e.g. you pass `github.example.com` instead of `https://github.example.com`), `https://` is assumed.
 * `--destination-token` - A [Personal Access Token](https://docs.github.com/en/enterprise/user/github/authenticating-to-github/creating-a-personal-access-token) for the destination GitHub Enterprise Server instance. If the destination repository is in an organization that does not yet exist or that you are not an owner of, your token will need to have the `site_admin` scope in order to create the organization or update the repository in it. The organization can also be created manually or an existing organization that you own can be used, in which case the `repo` and `workflow` scopes are sufficient. The token can also be provided by setting the `CODEQL_ACTION_SYNC_TOOL_DESTINATION_TOKEN` environment variable.
 
 **Optional Arguments:**

internal/pull/pull.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"os"
 	"regexp"
+	"sort"
 	"strings"
 
 	log "github.com/sirupsen/logrus"
@@ -50,6 +51,7 @@ type pullService struct {
 	assetOSIncludes        []string
 	assetOSExcludes        []string
 	assetCompressionFormat string
+	seenAssetOSs           map[string]bool
 }
 
 // shouldDownloadAsset determines whether a release asset should be downloaded, based on the
@@ -64,6 +66,11 @@ func (pullService *pullService) shouldDownloadAsset(assetName string) bool {
 	assetOS := matches[1]
 	assetCompressionFormat := matches[2]
 
+	if pullService.seenAssetOSs == nil {
+		pullService.seenAssetOSs = map[string]bool{}
+	}
+	pullService.seenAssetOSs[assetOS] = true
+
 	if len(pullService.assetOSIncludes) > 0 {
 		included := false
 		for _, os := range pullService.assetOSIncludes {
@@ -309,9 +316,36 @@ func (pullService *pullService) pullReleases() error {
 			}
 		}
 	}
+	pullService.warnOnUnmatchedOSFilters()
 	return nil
 }
 
+// warnOnUnmatchedOSFilters logs a warning if none of the configured --os-include or --os-exclude
+// values matched the OS identifier of any release asset that was actually encountered. This
+// helps surface typos or incorrect assumptions about asset OS identifiers (for example passing
+// "linux" when the real identifiers are "linux64"/"linux-arm64"), which would otherwise silently
+// filter out every OS-specific asset without any indication of why.
+func (pullService *pullService) warnOnUnmatchedOSFilters() {
+	seenList := make([]string, 0, len(pullService.seenAssetOSs))
+	for os := range pullService.seenAssetOSs {
+		seenList = append(seenList, os)
+	}
+	sort.Strings(seenList)
+
+	checkUnmatched := func(flagName string, values []string) {
+		for _, value := range values {
+			if !pullService.seenAssetOSs[value] {
+				log.Warnf(
+					"The %s value %q did not match any release asset. The OS identifiers found in the releases pulled were: %s.",
+					flagName, value, strings.Join(seenList, ", "),
+				)
+			}
+		}
+	}
+	checkUnmatched("--os-include", pullService.assetOSIncludes)
+	checkUnmatched("--os-exclude", pullService.assetOSExcludes)
+}
+
 func Pull(ctx context.Context, cacheDirectory cachedirectory.CacheDirectory, sourceToken string, sourceURL string, assetOSIncludes []string, assetOSExcludes []string, assetCompressionFormat string) error {
 	err := cacheDirectory.CheckOrCreateVersionFile(true, version.Version())
 	if err != nil {

internal/pull/pull_test.go

@@ -9,6 +9,8 @@ import (
 	"github.com/github/codeql-action-sync/internal/cachedirectory"
 	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing"
+	log "github.com/sirupsen/logrus"
+	logtest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/require"
 
 	"github.com/github/codeql-action-sync/test"
@@ -202,6 +204,36 @@ func TestShouldDownloadAsset(t *testing.T) {
 	}
 }
 
+func TestWarnOnUnmatchedOSFilters(t *testing.T) {
+	hook := logtest.NewGlobal()
+	defer hook.Reset()
+
+	pullService := pullService{
+		assetOSIncludes: []string{"linux", "win64"},
+		seenAssetOSs:    map[string]bool{"win64": true, "osx64": true},
+	}
+	pullService.warnOnUnmatchedOSFilters()
+
+	require.Len(t, hook.Entries, 1)
+	require.Equal(t, log.WarnLevel, hook.Entries[0].Level)
+	require.Contains(t, hook.Entries[0].Message, `The --os-include value "linux" did not match any release asset`)
+	require.Contains(t, hook.Entries[0].Message, "osx64, win64")
+}
+
+func TestWarnOnUnmatchedOSFiltersNoWarningWhenAllMatch(t *testing.T) {
+	hook := logtest.NewGlobal()
+	defer hook.Reset()
+
+	pullService := pullService{
+		assetOSIncludes: []string{"linux64"},
+		assetOSExcludes: []string{"win64"},
+		seenAssetOSs:    map[string]bool{"linux64": true, "win64": true, "osx64": true},
+	}
+	pullService.warnOnUnmatchedOSFilters()
+
+	require.Empty(t, hook.Entries)
+}
+
 func TestFindRelevantReleases(t *testing.T) {
 	temporaryDirectory := test.CreateTemporaryDirectory(t)
 	pullService := getTestPullService(t, temporaryDirectory, initialActionRepository, "")

internal/push/push.go

@@ -444,6 +444,10 @@ func Push(ctx context.Context, cacheDirectory cachedirectory.CacheDirectory, des
 	}
 
 	destinationURL = strings.TrimRight(destinationURL, "/")
+	if !strings.Contains(destinationURL, "://") {
+		log.Warnf("No protocol scheme specified in --destination-url %q, assuming https://.", destinationURL)
+		destinationURL = "https://" + destinationURL
+	}
 	token := oauth2.Token{AccessToken: destinationToken}
 	tokenSource := oauth2.StaticTokenSource(
 		&token,