Skip to content
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

[FR] allow setting bazel-version explicitly #21

Open
betaboon opened this issue Dec 6, 2024 · 4 comments
Open

[FR] allow setting bazel-version explicitly #21

betaboon opened this issue Dec 6, 2024 · 4 comments

Comments

@betaboon
Copy link
Contributor

betaboon commented Dec 6, 2024

Currently bazel.yaml set's two bazel-versions "implicitly", the version from .bazelversion and 8.0.0rc1. (source)

It would be great to be able to set the bazel-versions explicitly.

maybe something like this:

jobs:
  test:
    uses: bazel-contrib/.github/.github/workflows/bazel.yaml@v6
    with:
      bazelversions: |
        [
          "6.5.0",
          "7.4.1",
          "8.0.0rc1"
        ]

which could lead to the list being .bazelversion+those versions.

@betaboon
Copy link
Contributor Author

betaboon commented Dec 6, 2024

I've been trying with the following patch.
would you be open for a PR like this?

diff --git a/.github/workflows/bazel.yaml b/.github/workflows/bazel.yaml
index b68669d..b3715ab 100644
--- a/.github/workflows/bazel.yaml
+++ b/.github/workflows/bazel.yaml
@@ -21,6 +21,15 @@ on:
           JSON-formatted array of folders to run 'bazel test' in.
           For example, '[".", "e2e/smoke"]'
         type: string
+      bazelversions:
+        description: |
+          JSON-formatted array of bazelversion to run 'bazel test' for.
+
+          If this option is not set, the version from .bazelversion is used.
+
+          For example, '["7.4.1", "8.0.0rc1"]'
+        type: string
+        default: "[]"
       exclude:
         description: |
           JSON-formatted array of exclusions to the generated matrix of tests.
@@ -85,11 +94,10 @@ jobs:
       - uses: actions/checkout@v4
       # NB: we assume this is Bazel 7
       - id: bazel_from_bazelversion
+        if: inputs.bazelversions == '[]'
         run: echo "bazelversion=$(head -n 1 .bazelversion)" >> $GITHUB_OUTPUT
-      - id: bazel_8
-        run: echo "bazelversion=8.0.0rc1" >> $GITHUB_OUTPUT
     outputs:
-      # Will look like ["<version from .bazelversion>", "x.y.z"]
+      # Will look like ["<version from .bazelversion>"]
       bazelversions: ${{ toJSON(steps.*.outputs.bazelversion) }}
 
   test:
@@ -105,7 +113,11 @@ jobs:
       fail-fast: false
       matrix:
         os: ${{ fromJSON(needs.matrix-prep-os.outputs.os) }}
-        bazelversion: ${{ fromJSON(needs.matrix-prep-bazelversion.outputs.bazelversions) }}
+        bazelversion:
+          [
+            "${{ fromJSON(needs.matrix-prep-bazelversion.outputs.bazelversions) }}",
+            "${{ fromJSON(inputs.bazelversions) }}",
+          ]
         folder: ${{ fromJSON(inputs.folders) }}
         bzlmodEnabled: [true, false]
         exclude: ${{ fromJSON(inputs.exclude) }}

@betaboon
Copy link
Contributor Author

just to be explicit: I'd be willing to send a PR for this as long as this is a welcome change.

@alexeagle
Copy link
Contributor

Yes I think we should improve this. I myself tripped over it last week, when updating the .bazelversion in a project to 8.0.0.

I'd be a bit concerned about wasted time/resources if bazelrc and the workflow use different versions of a major, like .bazelversion has 8.0.1 while ci.yaml workflow has 8.0.0 - the user didn't mean to test both of those.

https://github.com/bazel-contrib/bazel-lib/blob/b2d88f07223ff097a13e4851ccca455f3b430d10/.github/workflows/ci.yaml#L25-L35 is a much more complex setup. It makes my eyes bleed even though I love bash. However it does solve for that issue and it's worth studying.

@betaboon
Copy link
Contributor Author

just to verify i understand:
assuming we allow declaring the tested bazelversions in the workflow, as described above, you're suggesting that the version from .bazelversion should overwrite the same major-version declared in the workflow?

if that's the case i see one downside tho:
if there are excludes in the workflow, that match one of the bazelversions declared in the workflow, they wouldn't apply anymore.
or we would need to do the same .bazelversion-overwrites-major-in-workflow thing.

also that assumes that testing for multiple minor- or patch-versions is not a usecase, which it might not be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants