On Mon, 19 Aug 2024 13:26:04 GMT, Magnus Ihse Bursie <[email protected]> wrote:
> Yes, this is somewhat more akin to what I was thinking. But I was actually
> thinking about a more encompassing rewrite of that method.
>
> First of all, GHA variables did not exist when that code were written, so
> they had to be secrets, even though it does not make sense in this context.
> These control variables should be changed from secrets to variables, or the
> code rewritten so it can support both.
>
> Secondly, I think there is a use-case for having both a variable which just
> appends to the default set of platforms/tests, and one that replaces the
> default value.
>
> Third, there has been requests to change what tests are run (e.g. tier-2) as
> well as which platforms are tested. I think it would be good to let such
> functionality tie in with any other changes in this area.
>
> But, just to be clear, I don't think it is necessary, nor even appropriate,
> for you to address any of these changes in this PR. In fact, I would have
> been happy with the previous iteration as well. But this version is okay as
> well, if you can move the exclude list to a more suited location.
yeah I totally agree with this idea, I'm happy to make these changes but that's
outside of the scope of this PR, I'll test some changes in my local environment.
> .github/workflows/main.yml line 84:
>
>> 82:
>> 83: # List of platforms to exclude by default
>> 84: EXCLUDED_PLATFORMS=("alpine-linux-x64")
>
> I'm not to fond of declaring a list like this in the middle of a code body.
> Perhaps you can set it up as a non-required workflow input with alpine as the
> default value, declared next to the default list of platforms?
yeah this makes sense, I'll make that change now
-------------
PR Comment: https://git.openjdk.org/jdk/pull/20577#issuecomment-2296595922
PR Review Comment: https://git.openjdk.org/jdk/pull/20577#discussion_r1721799725
