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.

Sign up for GitHub

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

Jump to bottom

#184: Sbt cross-build is broken #198

Merged
merged 12 commits into from
May 27, 2024
Merged

#184: Sbt cross-build is broken #198

merged 12 commits into from
May 27, 2024

Conversation

benedeki
Copy link
Contributor

@benedeki benedeki commented May 21, 2024 •
edited
Loading

  • build workflow script reworked to build, execute unit tests, execute integration tests and create code documentation
  • JaCoCo report workflow script put into one file and fixed to work
  • several other workflow scripts adjusted for easier reading by adding or adjusting job or file names

Closes #184

@benedeki benedeki added the work in progress Work on this item is not yet finished (mainly intended for PRs) label May 21, 2024
@benedeki benedeki self-assigned this May 21, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 21, 2024 •
edited
Loading

JaCoCo server module code coverage report - scala 2.13.11

Overall Project 70.78% 🍏

There is no coverage information present for the Files changed

@benedeki benedeki force-pushed the bugfix/184-sbt-cross-build-is-broken branch from a515913 to c8726d3 Compare May 23, 2024 14:02
lsulak
lsulak reviewed May 24, 2024
View reviewed changes
Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

LGTM so far! We'll need to adjust the required checks on the Repository Settings level as well, because these are not dynamically discovered as the code changes. I can help if you want.

@benedeki benedeki removed the work in progress Work on this item is not yet finished (mainly intended for PRs) label May 24, 2024
@github-actions GitHub Actions
Copy link

JaCoCo model module code coverage report - scala 2.13.11

Overall Project 62.5% 🍏

There is no coverage information present for the Files changed

@github-actions GitHub Actions
Copy link

github-actions bot commented May 24, 2024 •
edited
Loading

JaCoCo agent module code coverage report - scala 2.13.11

Overall Project 86.3% 🍏

There is no coverage information present for the Files changed

lsulak
lsulak reviewed May 24, 2024
View reviewed changes
.github/workflows/jacoco_report.yml Outdated
paths: ${{ github.workspace }}/model/target/jvm-${{ env.scalaShort }}/jacoco/report/jacoco.xml
token: ${{ secrets.GITHUB_TOKEN }}
min-coverage-overall: ${{ env.overall }}
min-coverage-changed-file: ${{ env.changed }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
min-coverage-changed-file: ${{ env.changed }}
min-coverage-changed-files: ${{ env.changed }}

I just opened https://github.com/AbsaOSS/atum-service/actions/runs/9227481018/job/25389516611?pr=198 and there is warning:

Warning: Unexpected input(s) 'name', 'min-coverage-changed-file', valid inputs are ['paths', 'token', 'min-coverage-overall', 'min-coverage-changed-files', 'title', 'update-comment', 'skip-if-no-changes', 'pass-emoji', 'fail-emoji', 'continue-on-error', 'debug-mode']

probably also name should be dropped

lsulak
lsulak reviewed May 24, 2024
View reviewed changes
.github/workflows/jacoco_report.yml Outdated
if: steps.jacocorun.outcome == 'success'
id: jacoco-agent
uses: madrapps/jacoco-report@v1.6.1
with:
name: agent-jacoco-report
paths: ${{ github.workspace }}/agent/target/jvm-${{ env.scalaShort }}/jacoco/report/jacoco.xml
paths: ${{ github.workspace }}/agent/target/spark3-jvm-${{ env.scalaShort }}/jacoco/report/jacoco.xml
token: ${{ secrets.GITHUB_TOKEN }}
min-coverage-overall: ${{ env.overall }}
min-coverage-changed-file: ${{ env.changed }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
min-coverage-changed-file: ${{ env.changed }}
min-coverage-changed-files: ${{ env.changed }}

I just opened https://github.com/AbsaOSS/atum-service/actions/runs/9227481018/job/25389516611?pr=198 and there is warning:

Warning: Unexpected input(s) 'name', 'min-coverage-changed-file', valid inputs are ['paths', 'token', 'min-coverage-overall', 'min-coverage-changed-files', 'title', 'update-comment', 'skip-if-no-changes', 'pass-emoji', 'fail-emoji', 'continue-on-error', 'debug-mode']

probably also name should be dropped

@miroslavpojer
Copy link
Collaborator

miroslavpojer commented May 27, 2024 •
edited
Loading

See 6 expected checks, which are visible. It should be removed.

@miroslavpojer
Copy link
Collaborator

miroslavpojer commented May 27, 2024 •
edited
Loading

In README.md:

  • Because there are some java level incompatibilities between Spark 2.4 and Spark 3.x whe build on Java 11+, we have to drop support for Spark 2.4. If you need the agent to work with Spark 2.4 follow these steps:

miroslavpojer
miroslavpojer reviewed May 27, 2024
View reviewed changes
Copy link
Collaborator

@miroslavpojer miroslavpojer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • pulled
  • code review
  • local machine test

During the review, I created 2 posts unrelated to the changed files.

TebaleloS
TebaleloS reviewed May 27, 2024
View reviewed changes
.github/workflows/jacoco_report.yml Outdated
paths: ${{ github.workspace }}/model/target/jvm-${{ env.scalaShort }}/jacoco/report/jacoco.xml
token: ${{ secrets.GITHUB_TOKEN }}
min-coverage-overall: ${{ env.overall }}
min-coverage-changed-file: ${{ env.changed }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just like Ladislav said, I think this min-coverage-changed-file should be changed to min-coverage-changed-files

TebaleloS
TebaleloS reviewed May 27, 2024
View reviewed changes
Copy link
Collaborator

@TebaleloS TebaleloS left a comment

Choose a reason for hiding this comment

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

I think this is not lacking anything, just the suggestions that Ladislav made, then it can be approved.

lsulak
lsulak approved these changes May 27, 2024
View reviewed changes
Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

Looks good now, I quickly scanned through all the CI logs and it seems good, no warnings left

@benedeki benedeki merged commit 190e2fb into master May 27, 2024
7 checks passed
@benedeki benedeki deleted the bugfix/184-sbt-cross-build-is-broken branch May 27, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miroslavpojer miroslavpojer miroslavpojer left review comments

@TebaleloS TebaleloS TebaleloS left review comments

@lsulak lsulak lsulak approved these changes

@Zejnilovic Zejnilovic Awaiting requested review from Zejnilovic Zejnilovic is a code owner

@dk1844 dk1844 Awaiting requested review from dk1844 dk1844 is a code owner

@salamonpavel salamonpavel Awaiting requested review from salamonpavel salamonpavel is a code owner

Assignees

@benedeki benedeki

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sbt cross-build is broken
4 participants
@benedeki @miroslavpojer @lsulak @TebaleloS