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

Ci/cache refactor and ci versions update #436

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eruizalo
Copy link
Collaborator

Description

Right now cache action is executing later than it should be, creating tons of caches and not using them. Now we are sharing the cache and avoiding the install action if it is not needed

Related Issue and dependencies

  • Resolves N/A
  • Depends on N/A

How Has This Been Tested?

  • This pull request contains appropriate tests?:
    • Tested in my own fork on "on push" event

@eruizalo eruizalo added dependencies Pull requests that update a dependency file CI/CD Continuous integration and continuous delivery labels Mar 14, 2024
@eruizalo eruizalo requested a review from a team as a code owner March 14, 2024 15:26
@github-actions github-actions bot added documentation Improvements or additions to documentation and removed dependencies Pull requests that update a dependency file labels Mar 14, 2024
Copy link

github-actions bot commented Mar 14, 2024

:octocat: This is an auto-generated comment created by:

  • Date : 2024-03-14 20:38:26 +0000 (UTC)
  • Workflow : PR comment
  • Job name : create_test_summary_report
  • Run : 8287283352
  • Commit : af81780 ci: [-] remove cleaning paths no longer exist
Actor Triggering actor Sender
eruizalo
eruizalo
eruizalo
eruizalo
eruizalo
eruizalo
Triggered by:

Test summary report 📊

Spark version testing
3.0.0 684 passed, 2 skipped
3.0.1 684 passed, 2 skipped
3.0.2 684 passed, 2 skipped
3.0 684 passed, 2 skipped
3.1.0 712 passed, 2 skipped
3.1.1 712 passed, 2 skipped
3.1.2 712 passed, 2 skipped
3.1 712 passed, 2 skipped
3.2.0 716 passed, 2 skipped
3.2.1 716 passed, 2 skipped
3.2.2 716 passed, 2 skipped
3.2 716 passed, 2 skipped
3.3.0 716 passed, 2 skipped
3.3.1 716 passed, 2 skipped
3.3.2 716 passed, 2 skipped
3.3.3 716 passed, 2 skipped
3.3 716 passed, 2 skipped
3.4.0 716 passed, 2 skipped
3.4.1 716 passed, 2 skipped
3.4 716 passed, 2 skipped
3.5.0 716 passed, 2 skipped
3.5 716 passed, 2 skipped

@eruizalo eruizalo force-pushed the ci/cache_refactor_and_ci_versions_update branch from eda5fbd to af81780 Compare March 14, 2024 16:10
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.70%. Comparing base (fee55fc) to head (af81780).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
+ Coverage   97.69%   97.70%   +0.01%     
==========================================
  Files          62       64       +2     
  Lines        1255     1259       +4     
  Branches       37       37              
==========================================
+ Hits         1226     1230       +4     
  Misses         29       29              
Flag Coverage Δ
spark-3.0.x 96.72% <ø> (+0.01%) ⬆️
spark-3.1.x 97.45% <ø> (+<0.01%) ⬆️
spark-3.2.x 97.67% <ø> (+0.01%) ⬆️
spark-3.3.x 97.67% <ø> (+0.01%) ⬆️
spark-3.4.x 97.67% <ø> (+0.01%) ⬆️
spark-3.5.x 97.67% <ø> (+0.01%) ⬆️

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

see 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

with:
extraKey: "${{ env.SPARK_VERSION }}"
jvm: adopt:8
Copy link
Collaborator Author

@eruizalo eruizalo Mar 14, 2024

Choose a reason for hiding this comment

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

something is not working fine here
before --> [info] welcome to sbt 1.9.9 (AdoptOpenJDK Java 1.8.0_292)
after --> [info] welcome to sbt 1.9.9 (Eclipse Adoptium Java 11.0.22)

Copy link
Member

Choose a reason for hiding this comment

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

Is using a different JVM but is it affecting something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought cache won't affect Java, but it seems if cache works Java is not set (we are now skipping the "install" step)

Copy link
Member

Choose a reason for hiding this comment

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

Could it be that you exchange the cache step with the prepare step? It's something that I see suspicious

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, it is supposed to execute cache before the install so you avoid this step, that should not be the problem. The problem is that if we match a cache we shouldn't install anything else, I mean this section apps: sbt scala scalafmt. As we don't need to install those apps because they should be provided by the cache, I ignored the step completely, but the step not only install those apps, but it set the java version as well (jvm: adopt:8). Because the whole step is now ignored if there is a cache hit, the java set is ignored as well, so I would probably add a new step only setting the java version if there is a cache hit... but it is kind of weird and I'm trying to determine if cache is needed at all, because our build times are not much better with the cache step

@eruizalo eruizalo marked this pull request as draft March 14, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Continuous integration and continuous delivery documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants