Skip to content

Feature: exclude architectures #240

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

Merged
merged 9 commits into from
Apr 28, 2025
Merged

Conversation

cap10morgan
Copy link
Collaborator

#238 revealed a blind spot in the exclusion code: architectures. This PR refactors & reorganizes some code and fixes that. It replaces the config/distro-architectures subsystem with the more generalized exclusion mechanism.

The only slightly tricky part is that for our variant generation & exclusion code, it makes sense to have a separate variant for each architecture. However, when we generate the manifest file (that tells the Docker Official Image folks what images to build from which dirs in this repo), we need to combine variants that only vary by architecture. So that's what this PR does.

Fixes #238

Copy link
Owner

@Quantisan Quantisan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Just a quick note: Have you thought about adding a test for merge-architecture? If it isn’t already covered.

Also, while exclude? is tested in core, it might be useful to create a unit test for variant/exclude? as well. This can serve as a practical example in addition to the docstring.

These are critical fns, so I'm a bit more nit picky.

@cap10morgan
Copy link
Collaborator Author

I'm still working on this. I dove into spec-based generative testing, but I think I'm going to crawl back out of that rabbit hole for one of the fns I started implementing it for. I will have some generative testing coming up though. :)

@cap10morgan
Copy link
Collaborator Author

I'm going to try to wrap this up soon. Sorry it has lingered so long!

@cap10morgan
Copy link
Collaborator Author

I'm making progress on this again. It required getting the test.chuck library running under Babashka, which required changes to Babashka itself, the instaparse pod, and test.chuck (😅) but I now have it generating test inputs from regexes. Getting these generators right was really giving me trouble, but test.chuck's string-from-regex generator is awesome.

Additional commits coming soon!

@cap10morgan cap10morgan force-pushed the feature/exclude-architectures branch from 5621a2f to 7773c83 Compare April 23, 2025 19:11
@cap10morgan
Copy link
Collaborator Author

OK @Quantisan at long last this should be good to re-review 🎉

@cap10morgan
Copy link
Collaborator Author

Once this lands we'll start publishing arm64 alpine variants for Java 21+!

@Quantisan
Copy link
Owner

Quantisan commented Apr 23, 2025 via email

Copy link
Owner

@Quantisan Quantisan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Focusing my review on the tests and high-level. Correct me if I'm wrong, it looks like the shift is replacing the basic test runner with Kaocha, and now using Clojure Spec's fdef definitions to drive generative testing (test.check) and runtime contract validation (orchestra). I interpret the goal as: leveraging specs for more robust, automated testing across the many inputs and better edge-case detection. In which case, LGTM!

@Quantisan
Copy link
Owner

Quantisan commented Apr 27, 2025

Focusing on all-tags-test as an arbitrary example, I found myself shuffling it a bit for my own understanding. Sharing for a diff perspective:

(deftest all-tags-test
 (testing "Generates specific tag format: <base-alias>-<jdk>-<build-tool>"
   (let [;; --- Shared configuration for these test scenarios ---
         build-tool "tools-deps"
         build-tool-version "1.11.1.1155"

         ;; --- Define the scenarios we want to test ---
         ;; Each map represents a specific context based on JDK version.
         test-scenarios [
                         {:jdk 11
                          :base-image "eclipse-temurin" ; Default base for pre-JDK21
                          :distro :ubuntu/jammy       ; Default distro for pre-JDK21
                          :desc "JDK 11 (pre-JDK21 defaults)"}

                         {:jdk 17
                          :base-image "eclipse-temurin" ; Default base for pre-JDK21
                          :distro :ubuntu/jammy       ; Default distro for pre-JDK21
                          :desc "JDK 17 (pre-JDK21 defaults)"}

                         {:jdk 21
                          :base-image "debian"          ; Default base for JDK21+
                          :distro :debian/bookworm    ; Default distro for JDK21+
                          :desc "JDK 21 (post-JDK21 defaults)"}]


         ;; --- Define the function to generate the expected tag for this specific format ---
         ;; This format omits the default distro and the build tool version.
         ;; It uses "temurin" as the base image alias even if the base image is "debian".
         generate-expected-tag (fn [jdk build-tool]
                                 (str "temurin-" jdk "-" build-tool))]

     ;; --- Iterate through scenarios and perform assertions ---
     (doseq [{:keys [jdk base-image distro desc]} test-scenarios]
       (testing desc
         (let [variant {:base-image         base-image
                        :jdk-version        jdk
                        :distro             distro ; This is the default, so it should be omitted in the generated tag
                        :build-tool         build-tool
                        :build-tool-version build-tool-version}
               expected-tag (generate-expected-tag jdk build-tool)
               all-generated-tags (set (all-tags variant))]

           (is (contains? all-generated-tags expected-tag)
               (str "Set of tags should contain '" expected-tag "'. "
                    "Variant tested: " (pr-str variant) ". "
                    "All generated tags: " (pr-str all-generated-tags)))))))))

@cap10morgan cap10morgan merged commit 52822dd into master Apr 28, 2025
2 checks passed
@cap10morgan cap10morgan deleted the feature/exclude-architectures branch April 28, 2025 17:24
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

Successfully merging this pull request may close these issues.

arm64 support for alpine
2 participants