-
Notifications
You must be signed in to change notification settings - Fork 28
Fix PRs from forks #556
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
base: main
Are you sure you want to change the base?
Fix PRs from forks #556
Changes from all commits
8ea7cd4
768b47d
f4f5a3a
a27af95
89cb708
fbd29c7
b87aab0
cfebaad
fb064dc
2be1b4b
6aebe45
5ad552c
210b98c
23de3b6
90df1ac
4a489ec
f28142e
14a460e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,17 @@ | ||
| name: pull_request | ||
| name: Code format check | ||
| on: pull_request | ||
| jobs: | ||
| codegen: | ||
| name: Code format check | ||
| runs-on: ubuntu-22.04 | ||
| steps: | ||
| - name: Cancel Previous Actions | ||
| uses: styfle/cancel-workflow-action@0.11.0 | ||
| uses: styfle/cancel-workflow-action@0.12.1 | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've upgraded each action as there doesn't appear to be any particular reason to use old actions |
||
| with: | ||
| access_token: ${{ github.token }} | ||
| - name: Free disk space | ||
| run: | | ||
| : Free disk space | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a fancy hack I've learned that makes reading GitHub action logs better |
||
| echo "Before clearing disk space:" | ||
| df -h | ||
|
|
||
|
|
@@ -27,11 +28,11 @@ jobs: | |
| echo "After clearing disk space:" | ||
| df -h | ||
| - name: Check out code into the Go module directory | ||
| uses: actions/checkout@v3 | ||
| uses: actions/checkout@v5 | ||
| with: | ||
| fetch-depth: 0 | ||
| - name: Set up Go | ||
| uses: actions/setup-go@v4 | ||
| uses: actions/setup-go@v6 | ||
| with: | ||
| go-version-file: "go.mod" | ||
| id: go | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| name: pull_request | ||
| name: Tests | ||
|
|
||
| on: | ||
| push: | ||
|
|
@@ -13,43 +13,59 @@ jobs: | |
| runs-on: ubuntu-22.04 | ||
| steps: | ||
| - name: Cancel Previous Runs | ||
| uses: styfle/cancel-workflow-action@0.11.0 | ||
| uses: styfle/cancel-workflow-action@0.12.1 | ||
| with: | ||
| access_token: ${{ github.token }} | ||
| - uses: actions/checkout@v3 | ||
| - uses: actions/checkout@v5 | ||
| with: | ||
| fetch-depth: 0 | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v4 | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: 3.8 | ||
| - uses: google-github-actions/setup-gcloud@a48b55b3b0eeaf77b6e1384aab737fbefe2085ac | ||
|
|
||
| - name: Gcloud login | ||
| if: ${{ env.has_auth }} | ||
| uses: google-github-actions/auth@v2 | ||
|
Comment on lines
-26
to
+29
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Google refactored their actions a long time ago so that the login step is done by this other action. |
||
| with: | ||
| version: '290.0.1' | ||
| project_id: ${{ secrets.GCP_PROJECT_ID }} | ||
| service_account_key: ${{ secrets.GCP_SA_KEY }} | ||
| export_default_credentials: true | ||
| name: Gcloud Login | ||
| credentials_json: ${{ secrets.GCP_SA_KEY }} | ||
| create_credentials_file: true | ||
| env: | ||
| has_auth: ${{ secrets.GCP_PROJECT_ID && secrets.GCP_SA_KEY && 1 }} | ||
|
Comment on lines
+28
to
+35
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In order to not try to auth when you don't have credentials, you have to bend over backwards because GitHub doesn't give you access to |
||
|
|
||
| - name: Install Trivy (latest) | ||
| run: | | ||
| : "Install Trivy (latest)" | ||
| TRIVY_VERSION=$(curl --silent "https://api.github.com/repos/aquasecurity/trivy/releases/latest" | grep '"tag_name":' | sed -E 's/.*"v([^"]+)".*/\1/') | ||
| echo Using Trivy v${TRIVY_VERSION} | ||
| wget https://github.com/aquasecurity/trivy/releases/download/v${TRIVY_VERSION}/trivy_${TRIVY_VERSION}_Linux-64bit.deb | ||
| sudo dpkg -i trivy_${TRIVY_VERSION}_Linux-64bit.deb | ||
| - name: Set up Go | ||
| uses: actions/setup-go@v4 | ||
| uses: actions/setup-go@v6 | ||
| with: | ||
| go-version-file: "go.mod" | ||
| - name: Ensure Google Cloud tests are runnable for main repository | ||
| if: github.event_name == 'push' && github.repository_owner == 'solo-io' && env.SKIP_GCLOUD_TESTS | ||
| shell: bash | ||
| run: | | ||
| echo ::error title=Misconfigured::GCP_SA_KEY is missing | ||
| exit 1000 | ||
| env: | ||
| SKIP_GCLOUD_TESTS: ${{ secrets.GCP_SA_KEY == '' && '1' || '' }} | ||
| - name: Run tests | ||
|
jsoref marked this conversation as resolved.
|
||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.CLOUDBUILD_GITHUB_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.CLOUDBUILD_GITHUB_TOKEN || github.token }} | ||
|
Comment on lines
-45
to
+58
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This gives us at least some token in the pull request from fork (or in a pull request in a fork). |
||
| SKIP_GCLOUD_TESTS: ${{ secrets.GCP_SA_KEY == '' && '1' || '' }} | ||
| HAS_CLOUDBUILD_GITHUB_TOKEN: ${{ secrets.CLOUDBUILD_GITHUB_TOKEN && '1' || '' }} | ||
|
jsoref marked this conversation as resolved.
|
||
| TEST_PKG: ./... # Run all tests | ||
| run: make test | ||
| - uses: testspace-com/setup-testspace@v1 | ||
| - name: Set up testspaceTestspace | ||
| uses: testspace-com/setup-testspace@v1 | ||
| with: | ||
| domain: solo-io.testspace.com | ||
| if: ${{ always() && github.event_name == 'push' && github.ref == 'refs/heads/master' }} | ||
| if: ${{ always() && github.event_name == 'push' && github.ref == 'refs/heads/master' && github.repository_owner == 'solo-io' }} | ||
| - name: Push result to Testspace server | ||
| run: | | ||
| testspace push --verbose "**/junit.xml" | ||
| if: ${{ always() && github.event_name == 'push' && github.ref == 'refs/heads/master' }} | ||
| if: ${{ always() && github.event_name == 'push' && github.ref == 'refs/heads/master' && github.repository_owner == 'solo-io' }} | ||
|
Comment on lines
+63
to
+71
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These steps are incredibly unlikely to work in forks, so let's save everyone's forks the cost of trying. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,7 +55,7 @@ clean: ## Clean any local assets | |
|
|
||
| GINKGO_VERSION ?= $(shell echo $(shell go list -m github.com/onsi/ginkgo/v2) | cut -d' ' -f2) | ||
| GINKGO_ENV ?= GOLANG_PROTOBUF_REGISTRATION_CONFLICT=ignore ACK_GINKGO_DEPRECATIONS=$(GINKGO_VERSION) | ||
| GINKGO_FLAGS ?= -v -tags=purego -compilers=4 -fail-fast -race -randomize-suites -randomize-all -skip-package=./installutils/kubeinstall,./debugutils/test | ||
| GINKGO_FLAGS ?= -v -tags=purego -compilers=4 $(shell [ -z "${CI}" ] && echo '-fail-fast') -race -randomize-suites -randomize-all -skip-package=./installutils/kubeinstall,./debugutils/test | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It took me a lot of round trips to GitHub to find and fix all of the items here. It would have been much less painful if
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a new ENV variable
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makefile magic is really something I'm happy to not have had to fight in my day-to-day work for over a decade. Getting |
||
| GINKGO_REPORT_FLAGS ?= --json-report=test-report.json --junit-report=junit.xml -output-dir=$(OUTPUT_DIR) | ||
| GINKGO_COVERAGE_FLAGS ?= --cover --covermode=atomic --coverprofile=coverage.cov | ||
| TEST_PKG ?= ./... # Default to run all tests | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| changelog: | ||
| - type: FIX | ||
| issueLink: https://github.com/solo-io/go-utils/issues/559 | ||
| description: Fixing handling of PRs from forks and cleanup github action versions | ||
| resolvesIssue: true | ||
|
jsoref marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ import ( | |
| "bytes" | ||
| "context" | ||
| "fmt" | ||
| "io/ioutil" | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been deprecated for ages and the version of golang used here supports the replacements. |
||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
@@ -415,7 +414,7 @@ closing | |
| ctx := context.Background() | ||
| client, err := githubutils.GetClient(ctx) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| hasChangelog, err := changelogutils.RefHasChangelog(ctx, client, "solo-io", "testrepo", "master") | ||
| hasChangelog, err := changelogutils.RefHasChangelog(ctx, client, "solo-io", "reporting-client", "master") | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No one outside of solo-io has access to |
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(hasChangelog).To(BeTrue()) | ||
| }) | ||
|
|
@@ -508,7 +507,7 @@ func createSubdirs(dir string, names ...string) error { | |
| } | ||
|
|
||
| func mustWriteTestDir() string { | ||
| tmpDir, err := ioutil.TempDir("", "changelog-test-") | ||
| tmpDir, err := os.MkdirTemp("", "changelog-test-") | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| return tmpDir | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package changelogutils_test | |
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "time" | ||
|
|
@@ -27,8 +28,6 @@ var _ = Describe("ReaderTest", func() { | |
|
|
||
| const ( | ||
| owner = "solo-io" | ||
| repo = "testrepo" | ||
| sha = "9065a9a84e286ea7f067f4fc240944b0a4d4c82a" | ||
| ) | ||
|
|
||
| var ( | ||
|
|
@@ -41,8 +40,22 @@ var _ = Describe("ReaderTest", func() { | |
| file = changelogutils.ChangelogFile{ | ||
| Entries: []*changelogutils.ChangelogEntry{&entry}, | ||
| } | ||
| log = "1.yaml" | ||
| repo = "testrepo" | ||
| sha = "9065a9a84e286ea7f067f4fc240944b0a4d4c82a" | ||
|
Comment on lines
+43
to
+45
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, I'm letting the testrepo flavor continue to exist, although I'd be inclined to just use the public repositories entirely for the normal stuff and reserve if for the special cases. |
||
| ) | ||
|
|
||
| if os.Getenv("HAS_CLOUDBUILD_GITHUB_TOKEN") == "" { | ||
| log = "new-signature-manager.yaml" | ||
| repo = "reporting-client" | ||
| sha = "af5d207720ee6b548704b06bfa6631f9a2897294" | ||
| entry = changelogutils.ChangelogEntry{ | ||
| Type: changelogutils.NEW_FEATURE, | ||
| Description: "New signature manager implementation to be used in CLI clients that writes the signature to ~/.soloio", | ||
| IssueLink: "https://github.com/solo-io/gloo/issues/1559", | ||
| } | ||
| } | ||
|
Comment on lines
+48
to
+57
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the general logic for replacing the private repo structure bits with things that work for everyone who isn't a member of solo-io. |
||
|
|
||
| BeforeEach(func() { | ||
| client, err := githubutils.GetClient(ctx) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
@@ -51,7 +64,7 @@ var _ = Describe("ReaderTest", func() { | |
| }) | ||
|
|
||
| It("can read changelog file", func() { | ||
| changelogFile, err := reader.ReadChangelogFile(ctx, "changelog/v0.1.1/1.yaml") | ||
| changelogFile, err := reader.ReadChangelogFile(ctx, fmt.Sprintf("changelog/v0.1.1/%s", log)) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since I found a similar v0.1.1, I'm using Sprintf to select the filename -- I could have left the entire string in the variables above, but this seemed slightly easier to understand. (Apparently |
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(*changelogFile).To(BeEquivalentTo(file)) | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package builders | |
|
|
||
| import ( | ||
| "context" | ||
| "os" | ||
| "path" | ||
|
|
||
| "cloud.google.com/go/storage" | ||
|
|
@@ -23,39 +24,41 @@ var _ = Describe("gcloud unit tests", func() { | |
| Expect(dir).To(Equal("gs://one/two/")) | ||
| }) | ||
|
|
||
| Context("builders", func() { | ||
| var builderCtx *mockBuilderContext | ||
| BeforeEach(func() { | ||
| var err error | ||
| builderCtx, err = DefaultMockBuilderContext(ctx) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
| Context("storage source", func() { | ||
| var ( | ||
| sb *StorageBuilder | ||
| ) | ||
|
|
||
| if os.Getenv("SKIP_GCLOUD_TESTS") == "" { | ||
| Context("builders", func() { | ||
|
Comment on lines
+27
to
+28
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This stuff needs Gcloud, and it's. incredibly dangerous to let foreign code run with credentials, so the safest thing to do is not to run it. |
||
| var builderCtx *mockBuilderContext | ||
| BeforeEach(func() { | ||
| var err error | ||
| client, err := storage.NewClient(ctx) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| sb = &StorageBuilder{ | ||
| client: client, | ||
| } | ||
| }) | ||
| It("can init build with sha", func() { | ||
| _, err := sb.InitBuildWithSha(ctx, builderCtx) | ||
| builderCtx, err = DefaultMockBuilderContext(ctx) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
| It("can init build with tag", func() { | ||
| _, err := sb.InitBuildWithTag(ctx, builderCtx) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Context("storage source", func() { | ||
| var ( | ||
| sb *StorageBuilder | ||
| ) | ||
|
|
||
| BeforeEach(func() { | ||
| var err error | ||
| client, err := storage.NewClient(ctx) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| sb = &StorageBuilder{ | ||
| client: client, | ||
| } | ||
| }) | ||
| It("can init build with sha", func() { | ||
| _, err := sb.InitBuildWithSha(ctx, builderCtx) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
| It("can init build with tag", func() { | ||
| _, err := sb.InitBuildWithTag(ctx, builderCtx) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| Context("repo source", func() { | ||
| Context("repo source", func() { | ||
|
|
||
| }) | ||
| }) | ||
| }) | ||
| } | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having two very unrelated workflows with the same name wasn't terribly helpful.