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

8094 Experimental CI Java 17 Maven Unittest build #8362

Merged
merged 7 commits into from
Feb 4, 2022

Conversation

poikilotherm
Copy link
Contributor

What this PR does / why we need it:

This adds an experimental build on the Github Workflows for IQSS/dataverse using Java 17. It's easier to recognise what fails with Java 17 if this becomes a part of regular CI.

Which issue(s) this PR closes:

Relates to #8094

Special notes for your reviewer:
Well take a look at the failing CI which should not fail the entire build (which is good)

Suggestions on how to test this:
See build log of the PR

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope, dev only.

Is there a release notes update needed for this change?:
Nope.

Additional documentation:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error

@poikilotherm poikilotherm added Type: Suggestion an idea Component: Code Infrastructure formerly "Feature: Code Infrastructure" labels Jan 24, 2022
@poikilotherm poikilotherm changed the title feat(ci): experimental Java 17 Maven Unittest build #8094 8094 Experimental CI Java 17 Maven Unittest build Jan 24, 2022
@poikilotherm poikilotherm force-pushed the 8094-java17-exp-ci branch 2 times, most recently from f26ebe5 to 2a3be52 Compare January 24, 2022 15:26
@coveralls
Copy link

coveralls commented Jan 24, 2022

Coverage Status

Coverage remained the same at 18.911% when pulling c297577 on poikilotherm:8094-java17-exp-ci into 0ea1f04 on IQSS:develop.

@poikilotherm poikilotherm force-pushed the 8094-java17-exp-ci branch 3 times, most recently from 81fa439 to c297577 Compare January 26, 2022 10:08
@poikilotherm poikilotherm marked this pull request as ready for review January 26, 2022 11:18
Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

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

Makes sense to start testing. Others are probably better able to comment on the implementation - from my view, if it works and isn't required, it's good.

@djbrooke djbrooke assigned djbrooke and scolapasta and unassigned djbrooke Jan 26, 2022
Copy link
Contributor

@scolapasta scolapasta left a comment

Choose a reason for hiding this comment

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

I like the idea of starting to test on Java 17 (and using this framework for other future versions too).

@scolapasta scolapasta removed their assignment Feb 3, 2022
@kcondon
Copy link
Contributor

kcondon commented Feb 3, 2022

@poikilotherm Please refresh from develop? Thanks!

@pdurbin
Copy link
Member

pdurbin commented Feb 3, 2022

Once this is merged does it mean we're going to see "Some checks were not successful" on all pull requests until we fix the code to support Java 17? I like the comforting feeling I get from seeing "All checks have passed". With some failures you have to get used to knowing which failures are expected. It would be nice if we could somehow have "All IMPORTANT checks have passed".

@poikilotherm
Copy link
Contributor Author

poikilotherm commented Feb 3, 2022

Please see this very PR for an example how this will look like.

There is no way to tell GitHub to just print a warning, as GitHub only supports two states: ✅ yay / ❌ nay

This was the exact reason why I added the statement of "Experimental" and "Stable" to the action titles.

@poikilotherm
Copy link
Contributor Author

poikilotherm commented Feb 4, 2022

@kcondon rebased on current develop.

When pulling from latest upstream, I had a strange conflict for src/main/java/edu/harvard/iq/dataverse/GuestbookResponseServiceBean.java - my local copy of develop couldn't be fast-forwarded for unanalysed reasons.

So I wanted to make it a clean branch here and rebased instead of merging, which needed a force-push obviously.
I dunno how you checkout a local copy of a PR branch, but it might be necessary to git fetch it and reset it in your local feature branch.

@kcondon kcondon self-assigned this Feb 4, 2022
@kcondon kcondon merged commit 8b838f3 into IQSS:develop Feb 4, 2022
@pdurbin
Copy link
Member

pdurbin commented Feb 7, 2022

Please see this very PR for an example how this will look like.

There is no way to tell GitHub to just print a warning, as GitHub only supports two states: ✅ yay / ❌ nay

This was the exact reason why I added the statement of "Experimental" and "Stable" to the action titles.

Now that this has been merged, I'll reiterate that I'm a not huge fan of how every single pull request will now show a red x on it like this:

Screen Shot 2022-02-07 at 10 12 18 AM

It means we've lost our ability to tell at a glance if a pull request looks ok.

Also, if you're familiar with broken windows theory, once one thing is broken, other things tend to also break and go unnoticed.

If we're going to work on Java 17 support in #8094 soon, great, let's fix that broken window. As of this writing, however, it hasn't been prioritized. Unless @poikilotherm is actively working on it? I see some commits there.

@poikilotherm
Copy link
Contributor Author

Looks like we are not alone with this... actions/runner#2347

I don't currently work on JDK 17. Modules #8395 first, so I can push #8320 forward with them (already doing this local).

@pdurbin pdurbin added this to the 5.10 milestone Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Code Infrastructure formerly "Feature: Code Infrastructure" Type: Suggestion an idea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants