Baunsgaard commented on PR #1806:
URL: https://github.com/apache/systemds/pull/1806#issuecomment-1519056092
> 1. Using ubuntu-latest could break reproducibility when new versions of
ubuntu are added. I'd replace all calls to ubuntu-latest with ubuntu-22.04.
Sure we can make this change.
Personally i kind of like that we automatically keep up to date tho, but
running both latest and 22.04 for all tests would be to much. If we can design
it in such a way that all tests run on 22.04 and a subset (maybe applications
only) run on latest then that would be cool, but only run the latest version if
latest is different than 22.04.
In general that all seems like to much work compared to just having
everything run on latest, also worth noting is that latest only update once
every 2 years. so not a big concern in my opinion.
> 2. The applications-tests do not use setup-java and therefore use the
default java installation of ubuntu-22.04, so if not replacing ubuntu-latest
with a fixed version, I'd at least want to add a fixed version with setup java,
but maybe even pin both a version for java and ubuntu.
This sounds like a bug on my side, feel free to make a minor PR for it.
> 3. Maybe also combine the functions, components and applications matricies
into a shared matrix in order to avoid their matrix test groups from diverging,
unless their matricies should remain different? I don't know enough about the
project to say if this step would make sense.
The reason they were split was to give nice names on the UI when the tests
were run to clearly indicate what tests were executed. the component.a** is not
really quickly understandable.
Maybe if you want to combine with point 2 as a minor thing, and combine all
in a single workflow to reduce the code duplication but we still want some way
if clearly understanding what test is run in each workflow.
>4. Retention period for artifacts, the uploaded test-coverage reports may
be saved for up to 400 days as artifacts. I don't think they have a ton of
value to keep around that long since they can also be regenerated. I'd consider
setting the retention long enough to make sure it is around at least a bit
longer than it takes to get something merged, but I don't know how long this is
for this project.
I think you did a fine medium of 30 days, my only concern is that the
artifact is 54.8 MB perhaps there is a way to compress this?
or/and set a dynamic retension of only last 30 days on main branch and 7 on
PRs?
This concerns me because it can grow very quickly with a few commits filling
way to much.
> It turns out that adding a custom status badge to the readme is a bit
annoying since it needs to be saved somewhere, we have a few options:
I think your option 3 is the one we want. there should be a way of doing
this, since all the other batches are based on this as well. But this would be
a followup project.
Finally a question.
How does it look, are we having a bad code coverage? --- i assume that it is
fairly low since we did not keep it in mind the last few years.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@systemds.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
