Consolidate the ci/ and hack/ directories #10589

timflannagan · 2025-02-07T16:28:27Z

Description

This PR is a follow-up to the #10498 issue and consolidates the ci/ and hack/ directories, removes any unrelated files or directories, etc. For any removed files, I added some commentary in the commit descriptions for why they were removed.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

Nested README(s) are nice, but in this case the README.md was overly prescriptive and no longer relevant from my perspective. Signed-off-by: timflannagan <[email protected]>

The tools.go file is commonly housed in the hack/ directory for various projects in the k8s ecosystem. Signed-off-by: timflannagan <[email protected]>

This file is no longer relevant with the introduction of the new release pipeline. Signed-off-by: timflannagan <[email protected]>

Signed-off-by: timflannagan <[email protected]>

This commit removes the check-go-version.sh bash script. From my perspective, this script is no longer needed as Go 1.21+ has been out for a while. Signed-off-by: timflannagan <[email protected]>

Signed-off-by: timflannagan <[email protected]>

nfuden · 2025-02-07T16:32:30Z

In my mind hack may include non-ci things versus ci is purely ci focussed.

is there any worry that we may make it harder to reason about what is strictly needed for ci purposes or is the stance that almost anything that we have is tested somehow so the difference between the two folders is unnecessary ?

This commit removes the github-actions directory from the repository. Previously, this directory housed the go-test-summary and process-skip-directive packages. With the want/need to shift away from the current changelog format from the gloo project, the latter is no longer relevant to this project. I'm less familiar with the former and whether it has any devex value that we should continue to maintain with this project. I think we're in the clear to remove it and re-introduce similiar functionality when we hit pain points as the project continues to mature. Signed-off-by: timflannagan <[email protected]>

timflannagan · 2025-02-07T16:53:55Z

We discussed the ci vs. hack briefly in the #10566, but here's where I think I'm at with the current structure :

The hack/ directory is widely adopted throughout the k8s ecosystem
Requires maintaining two root directories that contain misc. bash scripts, one-off Go packages, etc.
All GHAs should be ideally calling Makefile targets (vs. bash scripts directly) and storing those scripts in hack/ is very common / best practice
We'll have to outline which $things belong in ci vs. hack

We can discuss as a group what's best here. At a minimum, I think we'll remove some of the cruft in the ci/ directory if we don't want to move in this direction.

jenshu · 2025-02-07T20:35:37Z

.github/actions/kube-gateway-api-conformance-tests/action.yaml

@@ -36,7 +36,7 @@ runs:
      CLUSTER_NODE_VERSION: ${{ matrix.kube-version.node }}
      IMAGE_VARIANT: ${{ matrix.image-variant }}
      CONFORMANCE: "true"
-    run: ./ci/kind/setup-kind.sh
+    run: ./hack/hack/setup-kind.sh


wait why are there 2 hacks?

oh i see, it's a typo, should be hack/kind (saw a few other instances too)

Bad copy/paste from the airport lounge 😢. I'll push up some fixes next week.

jenshu · 2025-02-07T20:36:01Z

.github/actions/kubernetes-e2e-tests/action.yaml

-    # Print out a summary of ALL tests run under this action. In the future we can use this tool
-    # with the --json flag to export the results for centralized processing.
-    shell: bash
-    run: go run ./ci/github-actions/go-test-summary


why is this removed?

Added some context in the git commit message -- from my perspective, I think we can remove this and re-introduce over time if there's some pain points that this type of approach solves.

jenshu · 2025-02-07T20:36:14Z

.github/actions/kubernetes-e2e-tests/action.yaml

  - name: Archive bug report directory on failure
    if: ${{ failure() }}
    uses: actions/upload-artifact@v4
    with:
      name: bug-report-${{ inputs.cluster-name }}-${{ inputs.matrix-label }}-attempt-${{ github.run_attempt }}
      path: ./_test/bug_report/${{ inputs.cluster-name }}
-  - name: Archive test results


why is this removed?

jenshu · 2025-02-07T20:38:16Z

.github/actions/process-skip-directives/action.yaml

-outputs:
-  skip-kube-tests:
-    description: "Whether or not to skip the kube2e tests"
-    value: ${{ steps.process-skip-directives.outputs.skip-kube-tests }}


it can be useful to be able to skip regression tests (e.g. on PRs that only modify READMEs).
do we have another mechanism to skip tests, if we remove this?

There's some extension hooks that GH supports, but there's some nuance in how they behave. In the context of these changes, the skipCI-related directives aren't being leveraged and we're looking at overhauling the changelog process, so this effectively felt like cruft to me. WYDT?

jenshu · 2025-02-07T20:39:20Z

.github/workflows/pr-kubernetes-tests.yaml_noop

@@ -25,17 +23,6 @@
 #         # We require gathering the branch and tag history since we rely on a `git diff`
 #         # which compares the state of two branches
 #         fetch-depth: 0
-#     - id: process-skip-directives


related to above comment - are we sure we want to remove this now?

jenshu · 2025-02-07T20:42:25Z

Makefile

@@ -132,7 +132,7 @@ fmt-changed:

 # must be a separate target so that make waits for it to complete before moving on
 .PHONY: mod-download
-mod-download: check-go-version
+mod-download:


is check-go-version not useful anymore?

I don't think it's strictly needed anymore with Go 1.21+ being out for a while.

jenshu · 2025-02-07T20:43:09Z

.github/workflows/static-analysis.yaml

@@ -20,8 +20,7 @@ jobs:
    - name: Prep Go Runner
      uses: ./.github/actions/prep-go-runner
    - name: Generate Code
-      run: |
-        ./ci/check-generated-code.sh
+      run: make generate-all


should this be calling make verify instead?

jenshu · 2025-02-07T20:43:42Z

design/10498.md

@@ -89,7 +89,7 @@ make generate-all
 6. Run Full Validation & finalize the changes.

 ```bash
-export VERSION="v0.0.1"; CONFORMANCE="true" ./ci/kind/setup-kind.sh && helm upgrade -i -n kgateway-system kgateway _test/kgateway-$VERSION.tgz --create-namespace && make conformance
+export VERSION="v0.0.1"; CONFORMANCE="true" ./hack/hack/setup-kind.sh && helm upgrade -i -n kgateway-system kgateway _test/kgateway-$VERSION.tgz --create-namespace && make conformance


Suggested change

export VERSION="v0.0.1"; CONFORMANCE="true" ./hack/hack/setup-kind.sh && helm upgrade -i -n kgateway-system kgateway _test/kgateway-$VERSION.tgz --create-namespace && make conformance

export VERSION="v0.0.1"; CONFORMANCE="true" ./hack/kind/setup-kind.sh && helm upgrade -i -n kgateway-system kgateway _test/kgateway-$VERSION.tgz --create-namespace && make conformance

timflannagan added 6 commits February 7, 2025 16:10

ci: Remove README.md

981c920

Nested README(s) are nice, but in this case the README.md was overly prescriptive and no longer relevant from my perspective. Signed-off-by: timflannagan <[email protected]>

ci: Move tools.go -> hack/

60d77ac

The tools.go file is commonly housed in the hack/ directory for various projects in the k8s ecosystem. Signed-off-by: timflannagan <[email protected]>

ci: Remove unused upload_github_release_assets.go

8dabc6d

This file is no longer relevant with the introduction of the new release pipeline. Signed-off-by: timflannagan <[email protected]>

Remove the ci/check-generated-code.sh, add verify target, etc

876e31a

Signed-off-by: timflannagan <[email protected]>

Remove check-go-version.sh

15379b5

This commit removes the check-go-version.sh bash script. From my perspective, this script is no longer needed as Go 1.21+ has been out for a while. Signed-off-by: timflannagan <[email protected]>

*: Move ci/kind -> hack/

f04b904

Signed-off-by: timflannagan <[email protected]>

timflannagan force-pushed the chore/consolidate-ci-dir branch from 1706fdd to c7f504d Compare February 7, 2025 16:37

nfuden requested review from danehans and jenshu February 7, 2025 17:09

jenshu reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate the ci/ and hack/ directories #10589

Consolidate the ci/ and hack/ directories #10589

timflannagan commented Feb 7, 2025

nfuden commented Feb 7, 2025

timflannagan commented Feb 7, 2025

jenshu Feb 7, 2025

jenshu Feb 7, 2025

timflannagan Feb 7, 2025

jenshu Feb 7, 2025

timflannagan Feb 7, 2025

jenshu Feb 7, 2025

jenshu Feb 7, 2025

timflannagan Feb 7, 2025

jenshu Feb 7, 2025

jenshu Feb 7, 2025

timflannagan Feb 7, 2025

jenshu Feb 7, 2025

jenshu Feb 7, 2025

	export VERSION="v0.0.1"; CONFORMANCE="true" ./hack/hack/setup-kind.sh && helm upgrade -i -n kgateway-system kgateway _test/kgateway-$VERSION.tgz --create-namespace && make conformance
	export VERSION="v0.0.1"; CONFORMANCE="true" ./hack/kind/setup-kind.sh && helm upgrade -i -n kgateway-system kgateway _test/kgateway-$VERSION.tgz --create-namespace && make conformance

Consolidate the ci/ and hack/ directories #10589

Are you sure you want to change the base?

Consolidate the ci/ and hack/ directories #10589

Conversation

timflannagan commented Feb 7, 2025

Description

Checklist:

nfuden commented Feb 7, 2025

timflannagan commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment