-
Notifications
You must be signed in to change notification settings - Fork 25
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
Conversation
|
Waiting for approval from someone in the solo-io org to start testing. |
| @@ -1,16 +1,17 @@ | |||
| name: pull_request | |||
| name: Code format check | |||
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.
| steps: | ||
| - name: Cancel Previous Actions | ||
| uses: styfle/cancel-workflow-action@0.11.0 | ||
| uses: styfle/cancel-workflow-action@0.12.1 |
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.
I've upgraded each action as there doesn't appear to be any particular reason to use old actions
| access_token: ${{ github.token }} | ||
| - name: Free disk space | ||
| run: | | ||
| : Free disk space |
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.
This is a fancy hack I've learned that makes reading GitHub action logs better
| - uses: google-github-actions/setup-gcloud@a48b55b3b0eeaf77b6e1384aab737fbefe2085ac | ||
|
|
||
| - name: Gcloud login | ||
| if: ${{ env.has_auth }} | ||
| uses: google-github-actions/auth@v2 |
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.
Google refactored their actions a long time ago so that the login step is done by this other action.
| if: ${{ env.has_auth }} | ||
| uses: google-github-actions/auth@v2 | ||
| 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 }} |
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.
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 secrets in if
|
|
||
| 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)) |
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.
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 changelog/{rev}/ is a thing.)
| if os.Getenv("SKIP_GCLOUD_TESTS") == "" { | ||
| Context("builders", 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.
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.
| if os.Getenv("HAS_CLOUDBUILD_GITHUB_TOKEN") == "" { | ||
| repo = "reporting-client" | ||
| repoWithoutReleasesName = "unik-hub" | ||
| sha = "af5d207720ee6b548704b06bfa6631f9a2897294" | ||
| commitsInSha = 2 | ||
| commit1 = "7ef898bc3df32db0e1ed2dee70a838c955a7b422" | ||
| commit2 = "f47eacc21bd62e6bc8bb8954af0dc1817079af0d" | ||
| tagWithSha = "v0.1.2" | ||
| shaForTag = "a1c75ffaa40ea2b89368bfc338dc3f6f990b6df2" | ||
| } |
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.
a bunch of tests needed alternates. So these are the alternates
| Context("With write token", func() { | ||
| BeforeEach(func() { | ||
| if os.Getenv("HAS_CLOUDBUILD_GITHUB_TOKEN") == "" { | ||
| Skip("Needs write token") | ||
| } | ||
| }) |
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.
As with the Gcloud stuff, these need write tokens and you can't safely give out write tokens to random people, so they have to be skipped. This is a problem, but, there isn't really a better approach. If it matters, a member should make their own branch based on the contributor's branch and make a new PR from that.
| 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 |
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.
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 -fail-fast was omitted. So, let's omit it for CI builds.
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.
Can we add a new ENV variable GINKGO_FAIL_FAST_PARAM ?= --fail-fast that gets included here and overwritten by the CI env instead of inline shell?
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.
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 ?= and similar operations right is above my paygrade. You should have enough permission to push to this PR if you have a syntax that works, please feel free to do so.
ad4f1c2 to
fba93b3
Compare
|
Issues linked to changelog: |
|
Apologies for missing this - I no longer have write permissions in this repo. @sheidkamp would you be able to take a look? |
I can take a look, probably later in the week. BTW, go-utils has been removed as a direct dependency of kgateway |
sheidkamp
left a comment
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.
A couple notes.
Also validated in a fork of my own: sheidkamp#1
| 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 |
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.
Can we add a new ENV variable GINKGO_FAIL_FAST_PARAM ?= --fail-fast that gets included here and overwritten by the CI env instead of inline shell?
| 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 |
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.
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 ?= and similar operations right is above my paygrade. You should have enough permission to push to this PR if you have a syntax that works, please feel free to do so.
|
@rpunia1 / @sheidkamp ? |
|
@jsoref - can you
After that we should be gtg, the other suggestions were more nits. |
|
What do you mean "update the branch", and can you provide content for the changelog? |
|
By "update the branch" I mean merge in main or rebase on main, either with git or rull the dice with the button on the PR. Here's an example PR. https://github.com/solo-io/go-utils/blob/main/changelog/v0.9.9/fix-issues.yaml. Yours need to go into a new |
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Google split auth out of setup-gcloud... Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
|
Err there already was a changelog file, it just needed to be moved to the new directory. Rebased and moved. |
|
rerunning the tests |
sheidkamp
left a comment
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.
tests failing CI
|
Issues linked to changelog: |
Signed-off-by: Josh Soref <[email protected]>
BOT NOTES:
resolves Pull requests from forks aren't mergable #559