From 487ce20c36d90ea29b29ff0fdee1184cf2512f17 Mon Sep 17 00:00:00 2001 From: Jaz White Date: Mon, 5 Feb 2024 11:08:21 -0500 Subject: [PATCH] [sc-231506] fix: extinction checking not working correctly (#93) --- .ldignore | 1 + comments/comments.go | 8 ++-- config/config.go | 7 +-- diff/diff.go | 2 + internal/aliases/aliases.go | 24 +++++++---- internal/extinctions/extinctions.go | 12 +++++- internal/github_actions/actions.go | 40 ++++++++++++++--- internal/ldclient/flags.go | 3 ++ internal/references/builder_test.go | 66 +++++++++++++++++++++++++++++ main.go | 44 +++++++++++++------ search/search.go | 11 +++-- testdata/test | 2 +- 12 files changed, 177 insertions(+), 43 deletions(-) create mode 100644 internal/references/builder_test.go diff --git a/.ldignore b/.ldignore index c1984beb..0085e42e 100644 --- a/.ldignore +++ b/.ldignore @@ -1 +1,2 @@ testDir/*.js +vendor diff --git a/comments/comments.go b/comments/comments.go index 51d3bdfd..1a7f19a6 100644 --- a/comments/comments.go +++ b/comments/comments.go @@ -7,7 +7,6 @@ import ( "fmt" "html" "html/template" - "log" "reflect" "sort" "strings" @@ -18,6 +17,7 @@ import ( "github.com/google/go-github/github" ldapi "github.com/launchdarkly/api-client-go/v13" lcr "github.com/launchdarkly/find-code-references-in-pull-request/config" + gha "github.com/launchdarkly/find-code-references-in-pull-request/internal/github_actions" refs "github.com/launchdarkly/find-code-references-in-pull-request/internal/references" ) @@ -113,7 +113,7 @@ func BuildFlagComment(buildComment FlagComments, flagsRef refs.ReferenceSummary, hash := md5.Sum([]byte(postedComments)) if existingComment != nil && strings.Contains(*existingComment.Body, hex.EncodeToString(hash[:])) { - log.Println("comment already exists") + gha.Log("comment already exists") return "" } @@ -129,7 +129,7 @@ func ProcessFlags(flagsRef refs.ReferenceSummary, flags []ldapi.FeatureFlag, con idx, _ := find(flags, flagKey) createComment, err := githubFlagComment(flags[idx], flagAliases, true, false, config) if err != nil { - log.Println(err) + gha.LogError(err) } buildComment.CommentsAdded = append(buildComment.CommentsAdded, createComment) } @@ -144,7 +144,7 @@ func ProcessFlags(flagsRef refs.ReferenceSummary, flags []ldapi.FeatureFlag, con } removedComment, err := githubFlagComment(flags[idx], flagAliases, false, extinct, config) if err != nil { - log.Println(err) + gha.LogError(err) } buildComment.CommentsRemoved = append(buildComment.CommentsRemoved, removedComment) } diff --git a/config/config.go b/config/config.go index 510e19ba..cedd2280 100644 --- a/config/config.go +++ b/config/config.go @@ -3,13 +3,14 @@ package config import ( "context" "errors" - "fmt" "os" "strconv" "strings" "github.com/google/go-github/github" "golang.org/x/oauth2" + + gha "github.com/launchdarkly/find-code-references-in-pull-request/internal/github_actions" ) type Config struct { @@ -30,10 +31,10 @@ type Config struct { func ValidateInputandParse(ctx context.Context) (*Config, error) { // mask tokens if accessToken := os.Getenv("INPUT_ACCESS-TOKEN"); len(accessToken) > 0 { - fmt.Printf("::add-mask::%s\n", accessToken) + gha.MaskInput(accessToken) } if repoToken := os.Getenv("INPUT_REPO-TOKEN"); len(repoToken) > 0 { - fmt.Printf("::add-mask::%s\n", repoToken) + gha.MaskInput(repoToken) } // init config with defaults diff --git a/diff/diff.go b/diff/diff.go index 11848569..902d2a14 100644 --- a/diff/diff.go +++ b/diff/diff.go @@ -6,6 +6,7 @@ import ( "strings" i "github.com/launchdarkly/find-code-references-in-pull-request/ignore" + gha "github.com/launchdarkly/find-code-references-in-pull-request/internal/github_actions" refs "github.com/launchdarkly/find-code-references-in-pull-request/internal/references" diff_util "github.com/launchdarkly/find-code-references-in-pull-request/internal/utils/diff_util" "github.com/launchdarkly/ld-find-code-refs/v2/aliases" @@ -79,6 +80,7 @@ func ProcessDiffs(matcher lsearch.Matcher, contents []byte, builder *refs.Refere elementMatcher := matcher.Elements[0] for _, flagKey := range elementMatcher.FindMatches(line) { aliasMatches := elementMatcher.FindAliases(line, flagKey) + gha.Debug("Found (%s) reference to flag %s with aliases %v", op, flagKey, aliasMatches) builder.AddReference(flagKey, op, aliasMatches) } if builder.MaxReferences() { diff --git a/internal/aliases/aliases.go b/internal/aliases/aliases.go index 4253f11f..ace366af 100644 --- a/internal/aliases/aliases.go +++ b/internal/aliases/aliases.go @@ -4,29 +4,35 @@ import ( "github.com/launchdarkly/ld-find-code-refs/v2/aliases" "github.com/launchdarkly/ld-find-code-refs/v2/options" + gha "github.com/launchdarkly/find-code-references-in-pull-request/internal/github_actions" "github.com/launchdarkly/find-code-references-in-pull-request/internal/utils" ) // Generate aliases, making sure to identify aliases in the removed diff contents func GenerateAliases(opts options.Options, flagKeys []string, diffContents aliases.FileContentsMap) (map[string][]string, error) { + gha.Log("Generating aliases...") aliasesByFlagKey, err := aliases.GenerateAliases(flagKeys, opts.Aliases, opts.Dir) if err != nil { return nil, err } - filePatternAliases := getFilepatternAliases(opts.Aliases) - for _, flag := range flagKeys { - for _, alias := range filePatternAliases { - aliases, err := aliases.GenerateAliasesFromFilePattern(alias, flag, opts.Dir, diffContents) - if err != nil { - // skip aliases that fail to generate - continue + if len(diffContents) > 0 { + gha.Debug("Generating aliases for removed files...") + filePatternAliases := getFilepatternAliases(opts.Aliases) + for _, flag := range flagKeys { + for _, alias := range filePatternAliases { + aliases, err := aliases.GenerateAliasesFromFilePattern(alias, flag, opts.Dir, diffContents) + if err != nil { + // skip aliases that fail to generate + continue + } + aliasesByFlagKey[flag] = append(aliasesByFlagKey[flag], aliases...) } - aliasesByFlagKey[flag] = append(aliasesByFlagKey[flag], aliases...) + aliasesByFlagKey[flag] = utils.Dedupe(aliasesByFlagKey[flag]) } - aliasesByFlagKey[flag] = utils.Dedupe(aliasesByFlagKey[flag]) } + gha.Log("Finished generating aliases...") return aliasesByFlagKey, nil } diff --git a/internal/extinctions/extinctions.go b/internal/extinctions/extinctions.go index 75e35b52..a69a8b9c 100644 --- a/internal/extinctions/extinctions.go +++ b/internal/extinctions/extinctions.go @@ -1,6 +1,7 @@ package extinctions import ( + gha "github.com/launchdarkly/find-code-references-in-pull-request/internal/github_actions" refs "github.com/launchdarkly/find-code-references-in-pull-request/internal/references" "github.com/launchdarkly/find-code-references-in-pull-request/search" "github.com/launchdarkly/ld-find-code-refs/v2/options" @@ -8,23 +9,30 @@ import ( ) func CheckExtinctions(opts options.Options, builder *refs.ReferenceSummaryBuilder) error { - flagKeys := make([]string, 0, len(builder.RemovedFlagKeys())) + flagKeys := builder.RemovedFlagKeys() + if len(flagKeys) == 0 { + return nil + } + gha.StartLogGroup("Checking for extinctions...") + defer gha.EndLogGroup() matcher, err := search.GetMatcher(opts, flagKeys, nil) if err != nil { return err } + gha.Debug("Searching for any remaining references to %d removed flags...", len(flagKeys)) references, err := ld_search.SearchForRefs(opts.Dir, matcher) if err != nil { return err } + gha.Debug("Found %d references to removed flags", len(references)) for _, ref := range references { for _, hunk := range ref.Hunks { + gha.Debug("Flag '%s' is not extinct", hunk.FlagKey) builder.AddHeadFlag(hunk.FlagKey) } } - return nil } diff --git a/internal/github_actions/actions.go b/internal/github_actions/actions.go index 4f7434a8..5f89e543 100644 --- a/internal/github_actions/actions.go +++ b/internal/github_actions/actions.go @@ -2,16 +2,18 @@ package github_actions import ( "fmt" + "log" "os" ) -func SetOutputOrLogError(name, value string) { - if err := SetOutput(name, value); err != nil { - LogError("Failed to set outputs.%s\n", name) +func SetOutput(name, value string) { + if err := setOutput(name, value); err != nil { + SetError("Failed to set outputs.%s\n", name) } } -func SetOutput(name, value string) error { +func setOutput(name, value string) error { + Debug("setting output %s=%s", name, value) output := os.Getenv("GITHUB_OUTPUT") f, err := os.OpenFile(output, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) @@ -23,14 +25,38 @@ func SetOutput(name, value string) error { return err } -func LogNotice(format string, a ...any) { +func MaskInput(input string) { + fmt.Printf("::add-mask::%s\n", input) +} + +func Log(format string, a ...any) { + log.Println(fmt.Sprintf(format, a...)) +} + +func LogError(err error) { + log.Println(err) +} + +func SetNotice(format string, a ...any) { fmt.Printf("::notice::%s\n", fmt.Sprintf(format, a...)) } -func LogWarning(format string, a ...any) { +func SetWarning(format string, a ...any) { fmt.Printf("::warning::%s\n", fmt.Sprintf(format, a...)) } -func LogError(format string, a ...any) { +func SetError(format string, a ...any) { fmt.Printf("::error::%s\n", fmt.Sprintf(format, a...)) } + +func Debug(format string, a ...any) { + fmt.Printf("::debug::%s\n", fmt.Sprintf(format, a...)) +} + +func StartLogGroup(format string, a ...any) { + fmt.Printf("::group::%s\n", fmt.Sprintf(format, a...)) +} + +func EndLogGroup() { + fmt.Println("::endgroup::") +} diff --git a/internal/ldclient/flags.go b/internal/ldclient/flags.go index d828a823..27003004 100644 --- a/internal/ldclient/flags.go +++ b/internal/ldclient/flags.go @@ -8,11 +8,13 @@ import ( ldapi "github.com/launchdarkly/api-client-go/v13" lcr "github.com/launchdarkly/find-code-references-in-pull-request/config" + gha "github.com/launchdarkly/find-code-references-in-pull-request/internal/github_actions" "github.com/launchdarkly/find-code-references-in-pull-request/internal/version" "github.com/pkg/errors" ) func GetAllFlags(config *lcr.Config) ([]ldapi.FeatureFlag, error) { + gha.Debug("Fetching all flags for project") params := url.Values{} params.Add("env", config.LdEnvironment) activeFlags, err := getFlags(config, params) @@ -32,6 +34,7 @@ func GetAllFlags(config *lcr.Config) ([]ldapi.FeatureFlag, error) { flags = append(flags, archivedFlags...) } + gha.Debug("Fetched %d flags", len(flags)) return flags, nil } diff --git a/internal/references/builder_test.go b/internal/references/builder_test.go new file mode 100644 index 00000000..9e962071 --- /dev/null +++ b/internal/references/builder_test.go @@ -0,0 +1,66 @@ +package flags + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestBuilder_Build(t *testing.T) { + ref := ReferenceSummaryBuilder{ + flagsAdded: map[string][]string{ + "flag1": {"alias1"}, + "flag2": {"alias2"}, + }, + flagsRemoved: map[string][]string{ + "flag2": {}, + "flag3": {"alias3"}, + "flag4": {"alias4"}, + }, + flagsFoundAtHead: map[string]struct{}{ + "flag3": {}, + }, + foundFlags: map[string]struct{}{ + "flag1": {}, + "flag2": {}, + "flag3": {}, + "flag4": {}, + }, + includeExtinctions: true, + } + + built := ref.Build() + + assert.Len(t, built.FlagsAdded, 2) + assert.Len(t, built.FlagsRemoved, 2) + assert.Len(t, built.ExtinctFlags, 1) +} + +func TestBuilder_RemovedFlagKeys(t *testing.T) { + ref := ReferenceSummaryBuilder{ + flagsAdded: map[string][]string{ + "flag1": {"alias1"}, + "flag2": {"alias2"}, + }, + flagsRemoved: map[string][]string{ + "flag2": {}, + "flag3": {"alias3"}, + "flag4": {"alias4"}, + }, + flagsFoundAtHead: map[string]struct{}{ + "flag3": {}, + }, + foundFlags: map[string]struct{}{ + "flag1": {}, + "flag2": {}, + "flag3": {}, + "flag4": {}, + }, + includeExtinctions: true, + } + + keys := ref.RemovedFlagKeys() + + assert.Len(t, keys, 3) + assert.ElementsMatch(t, keys, []string{"flag2", "flag3", "flag4"}) +} diff --git a/main.go b/main.go index d6f81411..de4d3b21 100644 --- a/main.go +++ b/main.go @@ -3,7 +3,6 @@ package main import ( "context" "fmt" - "log" "net/http" "os" "sort" @@ -42,7 +41,7 @@ func main() { failExit(err) if len(flags) == 0 { - gha.LogNotice("No flags found in project %s", config.LdProject) + gha.SetNotice("No flags found in project %s", config.LdProject) os.Exit(0) } @@ -54,28 +53,36 @@ func main() { flagKeys = append(flagKeys, flag.Key) } + gha.StartLogGroup("Preprocessing diffs...") multiFiles, err := getDiffs(ctx, config, *event.PullRequest.Number) failExit(err) diffMap := ldiff.PreprocessDiffs(opts.Dir, multiFiles) matcher, err := search.GetMatcher(opts, flagKeys, diffMap) + gha.EndLogGroup() failExit(err) builder := references.NewReferenceSummaryBuilder(config.MaxFlags, config.CheckExtinctions) + gha.StartLogGroup("Scanning diff for references...") + gha.Log("Searching for %d flags", len(flagKeys)) for _, contents := range diffMap { ldiff.ProcessDiffs(matcher, contents, builder) } + gha.EndLogGroup() if config.CheckExtinctions { if err := extinctions.CheckExtinctions(opts, builder); err != nil { - gha.LogWarning("Error checking for extinct flags") - log.Println(err) + gha.SetWarning("Error checking for extinct flags") + gha.LogError(err) } } + + gha.Log("Summarizing results") flagsRef := builder.Build() // Add comment + gha.StartLogGroup("Processing comment...") existingComment := checkExistingComments(event, config, ctx) buildComment := ghc.ProcessFlags(flagsRef, flags, config) postedComments := ghc.BuildFlagComment(buildComment, flagsRef, existingComment) @@ -86,6 +93,7 @@ func main() { err = postGithubComment(ctx, flagsRef, config, existingComment, *event.PullRequest.Number, comment) } + gha.EndLogGroup() // Set outputs setOutputs(config, flagsRef) @@ -96,7 +104,7 @@ func main() { func checkExistingComments(event *github.PullRequestEvent, config *lcr.Config, ctx context.Context) *github.IssueComment { comments, _, err := config.GHClient.Issues.ListComments(ctx, config.Owner, config.Repo, *event.PullRequest.Number, nil) if err != nil { - log.Println(err) + gha.LogError(err) } for _, comment := range comments { @@ -148,6 +156,7 @@ func postGithubComment(ctx context.Context, flagsRef references.ReferenceSummary } func getDiffs(ctx context.Context, config *lcr.Config, prNumber int) ([]*diff.FileDiff, error) { + gha.Debug("Getting pull request diff...") rawOpts := github.RawOptions{Type: github.Diff} raw, resp, err := config.GHClient.PullRequests.GetRaw(ctx, config.Owner, config.Repo, prNumber, rawOpts) if err != nil { @@ -158,7 +167,14 @@ func getDiffs(ctx context.Context, config *lcr.Config, prNumber int) ([]*diff.Fi return nil, err } - return diff.ParseMultiFileDiff([]byte(raw)) + + multi, err := diff.ParseMultiFileDiff([]byte(raw)) + if err != nil { + return nil, err + } + gha.Debug("Got %d diff files", len(multi)) + + return multi, nil } // Get options from config. Note: dir will be set to workspace @@ -167,14 +183,14 @@ func getOptions(config *lcr.Config) (options.Options, error) { viper.Set("dir", config.Workspace) viper.Set("accessToken", config.ApiToken) - err := options.InitYAML() - if err != nil { - log.Println(err) + if err := options.InitYAML(); err != nil { + gha.LogError(err) } return options.GetOptions() } func setOutputs(config *lcr.Config, flagsRef references.ReferenceSummary) { + gha.Debug("Setting outputs...") flagsModified := flagsRef.AddedKeys() setOutputsForChangedFlags("modified", flagsModified) @@ -194,17 +210,17 @@ func setOutputs(config *lcr.Config, flagsRef references.ReferenceSummary) { func setOutputsForChangedFlags(modifier string, changedFlags []string) { count := len(changedFlags) - gha.SetOutputOrLogError(fmt.Sprintf("any-%s", modifier), fmt.Sprintf("%t", count > 0)) - gha.SetOutputOrLogError(fmt.Sprintf("%s-flags-count", modifier), fmt.Sprintf("%d", count)) + gha.SetOutput(fmt.Sprintf("any-%s", modifier), fmt.Sprintf("%t", count > 0)) + gha.SetOutput(fmt.Sprintf("%s-flags-count", modifier), fmt.Sprintf("%d", count)) sort.Strings(changedFlags) - gha.SetOutputOrLogError(fmt.Sprintf("%s-flags", modifier), strings.Join(changedFlags, " ")) + gha.SetOutput(fmt.Sprintf("%s-flags", modifier), strings.Join(changedFlags, " ")) } func failExit(err error) { if err != nil { - gha.LogError(err.Error()) - log.Println(err) + gha.LogError(err) + gha.SetError(err.Error()) os.Exit(1) } } diff --git a/search/search.go b/search/search.go index a4e6f2f7..50f04857 100644 --- a/search/search.go +++ b/search/search.go @@ -3,6 +3,7 @@ package search import ( "strings" + gha "github.com/launchdarkly/find-code-references-in-pull-request/internal/github_actions" laliases "github.com/launchdarkly/ld-find-code-refs/v2/aliases" "github.com/launchdarkly/ld-find-code-refs/v2/options" lsearch "github.com/launchdarkly/ld-find-code-refs/v2/search" @@ -10,16 +11,20 @@ import ( "github.com/launchdarkly/find-code-references-in-pull-request/internal/aliases" ) -func GetMatcher(opts options.Options, flagKeys []string, diffContents laliases.FileContentsMap) (matcher lsearch.Matcher, err error) { +func GetMatcher(opts options.Options, flagKeys []string, diffContents laliases.FileContentsMap) (lsearch.Matcher, error) { aliasesByFlagKey, err := aliases.GenerateAliases(opts, flagKeys, diffContents) if err != nil { return lsearch.Matcher{}, err } + for key, alias := range aliasesByFlagKey { + gha.Debug("Generated aliases for '%s': %v", key, alias) + } + delimiters := strings.Join(lsearch.GetDelimiters(opts), "") elements := make([]lsearch.ElementMatcher, 0, 1) - elements = append(elements, lsearch.NewElementMatcher(opts.ProjKey, opts.Dir, delimiters, flagKeys, aliasesByFlagKey)) - matcher = lsearch.Matcher{ + elements = append(elements, lsearch.NewElementMatcher(opts.ProjKey, "", delimiters, flagKeys, aliasesByFlagKey)) + matcher := lsearch.Matcher{ Elements: elements, } diff --git a/testdata/test b/testdata/test index 30fd7a89..047b05b1 100644 --- a/testdata/test +++ b/testdata/test @@ -2,8 +2,8 @@ show-widgets showWidgets show_widgets show-widgets -old-pricing-banner oldPricingBanner show_widgets oldPricingBanner mobile-app-promo-ios +mobile-app-promo-ios