From ed0f0d4c016ad4e1b905ee7759cce4b7ed685266 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Wed, 18 Dec 2024 20:36:40 +0100 Subject: [PATCH 1/4] fix(tests) improve pipeline run cancellation test - Add logging for pipeline run creation and cancellation - Update wait options for pipeline run creation - Replace check run status check with pipeline run status check - Ensure proper handling of pipeline run cancellation status --- test/github_pullrequest_test.go | 58 ++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 15 deletions(-) diff --git a/test/github_pullrequest_test.go b/test/github_pullrequest_test.go index ff51f21b8..23a4bd687 100644 --- a/test/github_pullrequest_test.go +++ b/test/github_pullrequest_test.go @@ -13,10 +13,13 @@ import ( "time" "github.com/google/go-github/v66/github" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" - tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github" "github.com/openshift-pipelines/pipelines-as-code/test/pkg/options" + twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait" "gotest.tools/v3/assert" "gotest.tools/v3/assert/cmp" @@ -224,13 +227,25 @@ func TestGithubSecondCancelInProgress(t *testing.T) { g.RunPullRequest(ctx, t) defer g.TearDown(ctx, t) + g.Cnx.Clients.Log.Infof("Waiting for one pipelinerun to be created") + waitOpts := twait.Opts{ + RepoName: g.TargetNamespace, + Namespace: g.TargetNamespace, + MinNumberStatus: 1, + PollTimeout: twait.DefaultTimeout, + TargetSHA: g.SHA, + } + err := twait.UntilPipelineRunCreated(ctx, g.Cnx.Clients, waitOpts) + assert.NilError(t, err) + time.Sleep(10 * time.Second) + g.Cnx.Clients.Log.Infof("Creating /retest on PullRequest") - _, _, err := g.Provider.Client.Issues.CreateComment(ctx, g.Options.Organization, g.Options.Repo, g.PRNumber, + _, _, err = g.Provider.Client.Issues.CreateComment(ctx, g.Options.Organization, g.Options.Repo, g.PRNumber, &github.IssueComment{Body: github.String("/retest")}) assert.NilError(t, err) g.Cnx.Clients.Log.Infof("Waiting for the two pipelinerun to be created") - waitOpts := twait.Opts{ + waitOpts = twait.Opts{ RepoName: g.TargetNamespace, Namespace: g.TargetNamespace, MinNumberStatus: 2, @@ -241,18 +256,31 @@ func TestGithubSecondCancelInProgress(t *testing.T) { assert.NilError(t, err) g.Cnx.Clients.Log.Infof("Sleeping for 10 seconds to let the pipelinerun to be canceled") - time.Sleep(10 * time.Second) - res, resp, err := g.Provider.Client.Checks.ListCheckRunsForRef(ctx, g.Options.Organization, g.Options.Repo, g.SHA, &github.ListCheckRunsOptions{ - AppID: g.Provider.ApplicationID, - ListOptions: github.ListOptions{}, - }) - assert.NilError(t, err) - assert.Equal(t, resp.StatusCode, 200) + i := 0 + foundCancelled := false + for i < 10 { + prs, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("%s=%s", keys.SHA, g.SHA), + }) + assert.NilError(t, err) - assert.Equal(t, res.CheckRuns[0].GetConclusion(), "cancelled") -} + for _, pr := range prs.Items { + if pr.GetStatusCondition() == nil { + continue + } + if pr.Status.Conditions[0].Reason == "Cancelled" { + g.Cnx.Clients.Log.Infof("PipelineRun %s has been canceled", pr.Name) + foundCancelled = true + break + } + } + if foundCancelled { + break + } -// Local Variables: -// compile-command: "go test -tags=e2e -v -info TestGithubPullRequest$ ." -// End: + time.Sleep(5 * time.Second) + i++ + } + assert.Assert(t, foundCancelled, "No Pipelines has been found cancedl in NS %s", g.TargetNamespace) +} From 59c1272750084261b0a9b3aef24e2d2af75ccf77 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 19 Dec 2024 11:45:44 +0100 Subject: [PATCH 2/4] fix(makefile): update test flags and add comments - Introduced DEFAULT_GO_TEST_FLAGS for default test flags - Added comments for vendor and other targets - Removed redundant test-unit targets - Updated test-e2e to use DEFAULT_GO_TEST_FLAGS - Added gitlint target for commit message linting - Minor formatting and cleanup --- Makefile | 58 ++++++++++++++++++++---------------------- hack/gh-workflow-ci.sh | 8 +++--- 2 files changed, 31 insertions(+), 35 deletions(-) diff --git a/Makefile b/Makefile index 94ebe7082..0a372c9e5 100644 --- a/Makefile +++ b/Makefile @@ -9,7 +9,8 @@ OUTPUT_DIR=bin GO = go TIMEOUT_UNIT = 20m TIMEOUT_E2E = 45m -GO_TEST_FLAGS += +DEFAULT_GO_TEST_FLAGS := -v -race -failfast +GO_TEST_FLAGS := SHELL := bash TOPDIR := $(shell git rev-parse --show-toplevel) @@ -37,7 +38,7 @@ help: ## print this help FORCE: .PHONY: vendor -vendor: +vendor: ## generate vendor directory @echo Generating vendor directory @go mod tidy -compat=1.17 && go mod vendor @@ -51,21 +52,15 @@ windows: ## compile windows binaries env GOOS=windows GOARCH=amd64 go build -mod=vendor $(FLAGS) -v -o ./bin/tkn-pac.exe ./cmd/tkn-pac/main.go ##@ Testing -TEST_UNIT_TARGETS := test-unit-verbose test-unit-race test-unit-failfast -test-unit-verbose: ARGS=-v -test-unit-failfast: ARGS=-failfast -test-unit-race: ARGS=-race -$(TEST_UNIT_TARGETS): test-unit +test: test-unit ## Run test-unit test-clean: ## Clean testcache @echo "Cleaning test cache" @go clean -testcache -.PHONY: $(TEST_UNIT_TARGETS) test test-unit +.PHONY: test test-unit test-no-cache: test-clean test-unit ## Run test-unit without caching -test: test-unit ## Run test-unit test-unit: ## Run unit tests @echo "Running unit tests..." - @set -o pipefail ; \ - $(GO) test $(GO_TEST_FLAGS) -timeout $(TIMEOUT_UNIT) $(ARGS) ./pkg/... | { grep -v 'no test files'; true; } + $(GO) test $(DEFAULT_GO_TEST_FLAGS) $(GO_TEST_FLAGS) -timeout $(TIMEOUT_UNIT) ./pkg/... .PHONY: test-e2e-cleanup test-e2e-cleanup: ## cleanup test e2e namespace/pr left open @@ -74,7 +69,7 @@ test-e2e-cleanup: ## cleanup test e2e namespace/pr left open .PHONY: test-e2e test-e2e: test-e2e-cleanup ## run e2e tests env GODEBUG=asynctimerchan=1 \ - go test -timeout $(TIMEOUT_E2E) -failfast -count=1 -tags=e2e -v $(GO_TEST_FLAGS) ./test + $(GO) test $(DEFAULT_GO_TEST_FLAGS) $(GO_TEST_FLAGS) -timeout $(TIMEOUT_E2E) -failfast -count=1 -tags=e2e ./test .PHONY: html-coverage html-coverage: ## generate html coverage @@ -119,6 +114,10 @@ lint-shell: ${SH_FILES} ## runs shellcheck on all python files @echo "Linting shell script files..." @shellcheck $(SH_FILES) +.PHONY: gitlint +gitlint: ## Run gitlint + @gitlint --commit "`git log --format=format:%H --no-merges -1`" --ignore "Merge branch" + .PHONY: pre-commit pre-commit: ## Run pre-commit hooks script manually @pre-commit run --all-files @@ -154,12 +153,12 @@ fix-golangci-lint: ## run golangci-lint and fix on all go files --fix @[[ -n `git status --porcelain` ]] && { echo "Go files has been cleaned 🧹. Cleaned Files: ";git status --porcelain ;} || echo "Go files are clean ✨" -.PHONY: fmt ## formats the GO code(excludes vendors dir) -fmt: +.PHONY: fmt +fmt: ## formats the GO code(excludes vendors dir) @go fmt `go list ./... | grep -v /vendor/` -.PHONY: fumpt ## formats the GO code with gofumpt(excludes vendors dir) -fumpt: +.PHONY: fumpt +fumpt: ## formats the GO code with gofumpt(excludes vendors dir) @find test pkg -name '*.go'|xargs -P4 $(GOFUMPT) -w -extra ##@ Local Development @@ -171,15 +170,6 @@ dev: ## deploys dev setup locally dev-redeploy: ## redeploy pac in local setup ./hack/dev/kind/install.sh -p -.PHONY: dev-docs -dev-docs: download-hugo ## preview live your docs with hugo - @$(HUGO_BIN) server -s docs/ & - if type -p xdg-open 2>/dev/null >/dev/null; then \ - xdg-open http://localhost:1313; \ - elif type -p open 2>/dev/null >/dev/null; then \ - open http://localhost:1313; \ - fi - ##@ Generated files check-generated: # check if all files that needs to be generated are generated @git status -uno |grep -E "modified:[ ]*(vendor/|.*.golden$)" && \ @@ -194,15 +184,23 @@ update-golden: ## run unit tests (updating golden files) .PHONY: generated generated: update-golden fumpt ## generate all files that needs to be generated +##@ Docs + .PHONY: download-hugo -download-hugo: +download-hugo: ## Download hugo software ./hack/download-hugo.sh $(HUGO_VERSION) $(TMPDIR)/hugo -.PHONY: gitlint -gitlint: ## Run gitlint - @gitlint --commit "`git log --format=format:%H --no-merges -1`" --ignore "Merge branch" +.PHONY: dev-docs +dev-docs: download-hugo ## preview live your docs with hugo + @$(HUGO_BIN) server -s docs/ & + if type -p xdg-open 2>/dev/null >/dev/null; then \ + xdg-open http://localhost:1313; \ + elif type -p open 2>/dev/null >/dev/null; then \ + open http://localhost:1313; \ + fi + +##@ Misc - .PHONY: clean clean: ## clean build artifacts rm -fR bin diff --git a/hack/gh-workflow-ci.sh b/hack/gh-workflow-ci.sh index 0051abccd..0e50aefea 100755 --- a/hack/gh-workflow-ci.sh +++ b/hack/gh-workflow-ci.sh @@ -37,8 +37,8 @@ create_second_github_app_controller_on_ghe() { local test_github_second_webhook_secret="${3}" if [[ -n "$(type -p apt)" ]]; then - apt update && - apt install -y python3-yaml + sudo apt update && + sudo apt install -y python3-yaml elif [[ -n "$(type -p dnf)" ]]; then dnf install -y python3-pyyaml else @@ -96,8 +96,6 @@ run_e2e_tests() { export TEST_GITHUB_PRIVATE_TASK_URL="https://github.com/openshift-pipelines/pipelines-as-code-e2e-tests-private/blob/main/remote_task.yaml" export TEST_GITHUB_PRIVATE_TASK_NAME="task-remote" - export GO_TEST_FLAGS="-v -race -failfast" - export TEST_BITBUCKET_CLOUD_API_URL=https://api.bitbucket.org/2.0 export TEST_BITBUCKET_CLOUD_E2E_REPOSITORY=cboudjna/pac-e2e-tests export TEST_BITBUCKET_CLOUD_TOKEN=${bitbucket_cloud_token} @@ -142,7 +140,7 @@ run_e2e_tests() { mapfile -t tests < <(get_tests "${target}") echo "About to run ${#tests[@]} tests: ${tests[*]}" # shellcheck disable=SC2001 - GO_TEST_FLAGS="-run \"$(echo "${tests[*]}" | sed 's/ /|/g')\"" make test-e2e + make test-e2e GO_TEST_FLAGS="-run \"$(echo "${tests[*]}" | sed 's/ /|/g')\"" } collect_logs() { From 86ffd1949ed0c468d1746cbc6efeebeb65fcff52 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Wed, 18 Dec 2024 17:42:19 +0100 Subject: [PATCH 3/4] fix: handle nil repo in GitHub event payloads We were getting a crash on the controller due to a nil pointer dereference when the repository was nil in the payload. This commit adds checks to ensure the repository is not nil before accessing its properties. TestSkippedEvent shows the issue but it wasn't detected that the controller was crashing and happily returned ok. --- pkg/provider/github/parse_payload.go | 15 +++++++++++++++ pkg/provider/github/parse_payload_test.go | 18 ++++++++++++++++++ test/invalid_event_test.go | 2 +- 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/pkg/provider/github/parse_payload.go b/pkg/provider/github/parse_payload.go index 586e67a11..2ff3bb6d8 100644 --- a/pkg/provider/github/parse_payload.go +++ b/pkg/provider/github/parse_payload.go @@ -254,6 +254,9 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt return nil, err } case *github.PushEvent: + if gitEvent.GetRepo() == nil { + return nil, errors.New("error parsing payload the repository should not be nil") + } processedEvent.Organization = gitEvent.GetRepo().GetOwner().GetLogin() processedEvent.Repository = gitEvent.GetRepo().GetName() processedEvent.DefaultBranch = gitEvent.GetRepo().GetDefaultBranch() @@ -274,6 +277,9 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt processedEvent.HeadURL = processedEvent.BaseURL // in push events Head URL is the same as BaseURL case *github.PullRequestEvent: processedEvent.Repository = gitEvent.GetRepo().GetName() + if gitEvent.GetRepo() == nil { + return nil, errors.New("error parsing payload the repository should not be nil") + } processedEvent.Organization = gitEvent.GetRepo().Owner.GetLogin() processedEvent.DefaultBranch = gitEvent.GetRepo().GetDefaultBranch() processedEvent.SHA = gitEvent.GetPullRequest().Head.GetSHA() @@ -306,6 +312,9 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt func (v *Provider) handleReRequestEvent(ctx context.Context, event *github.CheckRunEvent) (*info.Event, error) { runevent := info.NewEvent() + if event.GetRepo() == nil { + return nil, errors.New("error parsing payload the repository should not be nil") + } runevent.Organization = event.GetRepo().GetOwner().GetLogin() runevent.Repository = event.GetRepo().GetName() runevent.URL = event.GetRepo().GetHTMLURL() @@ -331,6 +340,9 @@ func (v *Provider) handleReRequestEvent(ctx context.Context, event *github.Check func (v *Provider) handleCheckSuites(ctx context.Context, event *github.CheckSuiteEvent) (*info.Event, error) { runevent := info.NewEvent() + if event.GetRepo() == nil { + return nil, errors.New("error parsing payload the repository should not be nil") + } runevent.Organization = event.GetRepo().GetOwner().GetLogin() runevent.Repository = event.GetRepo().GetName() runevent.URL = event.GetRepo().GetHTMLURL() @@ -393,6 +405,9 @@ func (v *Provider) handleIssueCommentEvent(ctx context.Context, event *github.Is func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.CommitCommentEvent) (*info.Event, error) { action := "push" runevent := info.NewEvent() + if event.GetRepo() == nil { + return nil, errors.New("error parsing payload the repository should not be nil") + } runevent.Organization = event.GetRepo().GetOwner().GetLogin() runevent.Repository = event.GetRepo().GetName() runevent.Sender = event.GetSender().GetLogin() diff --git a/pkg/provider/github/parse_payload_test.go b/pkg/provider/github/parse_payload_test.go index b6cbbc89f..ab827becc 100644 --- a/pkg/provider/github/parse_payload_test.go +++ b/pkg/provider/github/parse_payload_test.go @@ -20,6 +20,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" ghtesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/github" "github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger" @@ -88,6 +89,9 @@ var samplePRAnother = github.PullRequest{ } func TestParsePayLoad(t *testing.T) { + samplePRNoRepo := samplePRevent + samplePRNoRepo.Repo = nil + tests := []struct { name string wantErrString string @@ -185,6 +189,20 @@ func TestParsePayLoad(t *testing.T) { }, shaRet: "samplePRsha", }, + { + name: "bad/pull request", + eventType: "pull_request", + triggerTarget: triggertype.PullRequest.String(), + payloadEventStruct: samplePRNoRepo, + wantErrString: "error parsing payload the repository should not be nil", + }, + { + name: "bad/push", + eventType: "push", + triggerTarget: triggertype.Push.String(), + payloadEventStruct: github.PushEvent{}, + wantErrString: "error parsing payload the repository should not be nil", + }, { // specific run from a check_suite name: "good/rerequest check_run on pull request", diff --git a/test/invalid_event_test.go b/test/invalid_event_test.go index 0de2ff53e..9fb634853 100644 --- a/test/invalid_event_test.go +++ b/test/invalid_event_test.go @@ -82,7 +82,7 @@ func TestSkippedEvent(t *testing.T) { assert.NilError(t, err) defer resp.Body.Close() - assert.Equal(t, resp.StatusCode, http.StatusOK, "%s reply expected 200 OK", elURL) + assert.Assert(t, resp.StatusCode >= 200 && resp.StatusCode < 300, "%s reply expected 2xx OK: %d", elURL, resp.StatusCode) } func TestGETCall(t *testing.T) { From 7bf19a04eaf3a051c0d67fbf27fad40b75e2d1a1 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 19 Dec 2024 14:34:01 +0100 Subject: [PATCH 4/4] show only the first 80 lines after a panic printing the whole log file is not useful, as it can be very long to scoll at times. --- hack/gh-workflow-ci.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hack/gh-workflow-ci.sh b/hack/gh-workflow-ci.sh index 0e50aefea..5e51aa2f0 100755 --- a/hack/gh-workflow-ci.sh +++ b/hack/gh-workflow-ci.sh @@ -37,7 +37,7 @@ create_second_github_app_controller_on_ghe() { local test_github_second_webhook_secret="${3}" if [[ -n "$(type -p apt)" ]]; then - sudo apt update && + sudo apt update && sudo apt install -y python3-yaml elif [[ -n "$(type -p dnf)" ]]; then dnf install -y python3-pyyaml @@ -140,7 +140,7 @@ run_e2e_tests() { mapfile -t tests < <(get_tests "${target}") echo "About to run ${#tests[@]} tests: ${tests[*]}" # shellcheck disable=SC2001 - make test-e2e GO_TEST_FLAGS="-run \"$(echo "${tests[*]}" | sed 's/ /|/g')\"" + make test-e2e GO_TEST_FLAGS="-run \"$(echo "${tests[*]}" | sed 's/ /|/g')\"" } collect_logs() { @@ -173,7 +173,7 @@ collect_logs() { detect_panic() { # shellcheck disable=SC2016 - (find /tmp/logs/ -type f -regex '.*/pipelines-as-code.*/[0-9]\.log$' | xargs -r sed -n '/stderr F panic:.*/,$p') >/tmp/panic.log + (find /tmp/logs/ -type f -regex '.*/pipelines-as-code.*/[0-9]\.log$' | xargs -r sed -n '/stderr F panic:.*/,$p' | head -n 80) >/tmp/panic.log if [[ -s /tmp/panic.log ]]; then set +x echo "===================== PANIC DETECTED ====================="