search

MovingBlocks / gestalt

A family of libraries providing a variety of core capabilities for use by games and game engines.
Apache License 2.0
27 stars 23 forks source link

ci: adjust desired agent label light-java -> light #138

Closed jdrueckert closed 1 year ago

jdrueckert commented 1 year ago

seems like the agent labels have changed over time. previously light-java is now called light

we might need java version specific label in addition. for instance, the current jenkins agent is jdk11. if we require jdk8 still, then we need to add a respective label. ideally jdk11 should be fine, potentially we need another build param for language level

jdrueckert commented 1 year ago

We indeed do not necessarily need this PR - not sure why the build job for #137 hung, it works now. However, I'd still argue that dropping light-java in favor of separate labels light and java (or specific java version respectively) makes sense as they're two different dimensions. @BenjaminAmos from your comment I'm not able to deduce whether you don't want to merge this or want me to change anything?

BenjaminAmos commented 1 year ago

The changes here are fine. Separating the light and java labels seems sensible. I was just answering the question (or what I saw as a question) in the pull request description regarding whether we need JDK 8 anymore. My answer was that we don't need JDK 8 agents for anything anymore since Destination Sol has required JDK 11 to build since earlier this year.

jdrueckert commented 1 year ago

I was just answering the question (or what I saw as a question) in the pull request description regarding whether we need JDK 8 anymore

Gotcha :+1:

My answer was that we don't need JDK 8 agents for anything anymore since Destination Sol has required JDK 11 to build since earlier this year.

Yup I got that :slightly_smiling_face: I also added a comment on my PR on @Cervator's artifactory fun project questioning whether we still need a JDK8 agent at all anymore. There might still be some part of our projects that needs it, but I don't know.

The changes here are fine

Would you care to approve then, so we can merge this? Or do you see any blockers?