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

Replace deprecated CircleCI image #19423

Merged
merged 3 commits into from
May 16, 2022
Merged

Replace deprecated CircleCI image #19423

merged 3 commits into from
May 16, 2022

Conversation

paoliniluis
Copy link
Contributor

@paoliniluis paoliniluis commented Dec 19, 2021

Fixes #18650

EDIT 04-16-2022: upgraded Clojure CLI version + made the builder image multi-arch

Tests on https://app.circleci.com/pipelines/github/metabase/metabase?branch=check-new-builder&filter=all

@codecov
Copy link

codecov bot commented Dec 19, 2021

Codecov Report

Merging #19423 (9ebb17e) into master (3d82910) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #19423      +/-   ##
==========================================
- Coverage   64.26%   64.25%   -0.01%     
==========================================
  Files        2478     2479       +1     
  Lines       81612    81608       -4     
  Branches     9980     9977       -3     
==========================================
- Hits        52445    52438       -7     
- Misses      25049    25054       +5     
+ Partials     4118     4116       -2     
Flag Coverage Δ
back-end 85.54% <ø> (-0.03%) ⬇️
front-end 44.16% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/metabase/db/liquibase/h2.clj 45.45% <0.00%> (-40.91%) ⬇️
src/metabase/models/query.clj 89.79% <0.00%> (ø)
...d/src/metabase/entities/collections/collections.js 91.07% <0.00%> (ø)
...ard/components/add-card-sidebar/QuestionPicker.jsx 0.00% <0.00%> (ø)
frontend/src/metabase/lib/collections.ts 57.14% <0.00%> (ø)
frontend/src/metabase/containers/ItemPicker.jsx 63.51% <0.00%> (+1.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d82910...9ebb17e. Read the comment docs.

@paoliniluis
Copy link
Contributor Author

paoliniluis commented Dec 20, 2021

I'm next to the finish line:

  1. image is built from new Temurin JDK + Node LTS + Clojure (8, 11 and 17)
  2. user profile is configured as circleci wants, so it's plug and play with the current CI (caching works!)
  3. it's slimmer (only ~600mb compared to current circleci image)
  4. it's very very upgradable (in fact I'm thinking into building one to build and other to test, so it's not a full image for every step)

but I can't get yarn to spawn a java process... but I'm sure it's a path thing somewhere that I'll have to fix (EDIT: it was a missing path in the spawn process)

EDIT2: I modified one of the runners from Java 16 to the latest 17 LTS

@paoliniluis paoliniluis marked this pull request as draft December 20, 2021 03:34
@paoliniluis paoliniluis marked this pull request as ready for review December 21, 2021 16:01
@paoliniluis
Copy link
Contributor Author

"be-tests-ee" and "be-tests-oss" are pending since those were changed "be-tests-java-8-ee" "be-tests-java-8-oss" to know which java version we're running those tests

@paoliniluis paoliniluis marked this pull request as draft December 31, 2021 02:09
@paoliniluis paoliniluis marked this pull request as ready for review December 31, 2021 20:00
@paoliniluis
Copy link
Contributor Author

Now this should also be ready to merge once all tests pass

.circleci/config.yml Outdated Show resolved Hide resolved

FROM eclipse-temurin:11-jre-alpine as runner
FROM --platform=linux/amd64 eclipse-temurin:11-jre-alpine as runner
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

@paoliniluis
Copy link
Contributor Author

Thanks @diogormendes for the approval, I'll need metabase/metabase-docker-ci#6 approved before merging this one

Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

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

We shouldn't test with Clojure 1.11.0 since we're not shipping anything with it yet.

We'll probably switch to Clojure 1.11.1 when that comes out but we're still shipping 1.10.x until then

@dpsutton
Copy link
Contributor

My reading of this is its just bumping the version of the clojure cli. We specify org.clojure/clojure {:mvn/version "1.10.3"} so we will not be testing on clojure 1.11.X at all. But we for sure want to stay away from 1.11.0 and only use 1.11.1 and above when we do switch.

@paoliniluis
Copy link
Contributor Author

Thanks @dpsutton and @camsaul, I left my comments here: metabase/metabase-docker-ci#6 (comment)

Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

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

Oops you're totally right @dpsutton. Ok in that case then this looks fine

@paoliniluis paoliniluis merged commit 7d2d862 into master May 16, 2022
@paoliniluis paoliniluis deleted the check-new-builder branch May 16, 2022 15:09
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.

Move away from CircleCI legacy images (deprecation)
5 participants