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

fix(buildPlugin) ensure the custom label is passed to node when adding custom platform #658

Merged
merged 1 commit into from
Apr 26, 2023
Merged

fix(buildPlugin) ensure the custom label is passed to node when adding custom platform #658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions test/groovy/BuildPluginStepTests.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,45 @@ class BuildPluginStepTests extends BaseTest {
// when running without any parameters
script.call([:])
printCallStack()
// then it runs in a linux node
assertTrue(assertMethodCallContainsPattern('node', 'linux'))
// then it runs in a windows node
assertTrue(assertMethodCallContainsPattern('node', 'windows'))
// then it runs a stage in a linux VM by default
assertTrue(assertMethodCallContainsPattern('node', 'vm && linux'))
// then it runs a stage in a Windows VM by default
assertTrue(assertMethodCallContainsPattern('node', 'docker-windows'))
// then it runs the junit step by default
assertTrue(assertMethodCall('junit'))
// then it runs the junit step with the maven test format
assertTrue(assertMethodCallContainsPattern('junit', '**/target/surefire-reports/**/*.xml,**/target/failsafe-reports/**/*.xml'))
assertJobStatusSuccess()
}

@Test
void test_buildPlugin_with_container_agents() throws Exception {
def script = loadScript(scriptName)
// when running with useContainerAgent set to true
script.call([useContainerAgent: true])
printCallStack()
// then it runs a stage in a linux container by default
assertTrue(assertMethodCallContainsPattern('node', 'maven'))
// then it runs a stage in a Windows container by default
assertTrue(assertMethodCallContainsPattern('node', 'maven-windows'))
assertJobStatusSuccess()
}

@Test
void test_buildPlugin_with_customplatforms() throws Exception {
def script = loadScript(scriptName)
// when running with useContainerAgent set to true
script.call(platforms: ['openbsd', 'maven-windows-experimental'])
printCallStack()
// then it runs a stage in an openbsd node with a warning message
assertTrue(assertMethodCallContainsPattern('node', 'openbsd'))
assertTrue(assertMethodCallContainsPattern('echo', 'WARNING: Unknown Virtual Machine platform \'openbsd\''))
// then it runs a stage in a maven-windows-experimental node with a warning message
assertTrue(assertMethodCallContainsPattern('node', 'maven-windows-experimental'))
assertTrue(assertMethodCallContainsPattern('echo', 'WARNING: Unknown Virtual Machine platform \'maven-windows-experimental\''))
assertJobStatusSuccess()
}

@Test
void test_buildPlugin_with_timeout() throws Exception {
def script = loadScript(scriptName)
Expand Down
4 changes: 2 additions & 2 deletions vars/buildPlugin.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ def call(Map params = [:]) {
label = 'vm && linux'
break
default:
echo "WARNING: Unknown platform '${platform}'. Agent label set to fallback value 'linux'"
label = 'linux'
echo "WARNING: Unknown Virtual Machine platform '${platform}'. Set useContainerAgent to 'true' unless you want to be in uncharted territory."
Copy link
Contributor

Choose a reason for hiding this comment

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

this still seems at odds with the documentation (ie this is supposed to support labels - so using a label (even a label expression if I read this doc correct) should be ok here and not produce a warning?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, updating the documentation is the next step 👍 thanks for pointing out, I would have forgotten to be honest.

We are trying to update the label models because the current one is not usable in 2023. Some examples:

  • Should you expect docker to be available when specifying maven-windows (no obvious answer of course :D). Which JDK should you have?
  • When you say linux, do you mean x86 or arm64? With or without Docker Engine?

=> our current strategy is to move to explicit tags, #522 was driven by this idea. Becasue most of the labels are specified by the pipeline library, only edge cases uses custom labels.

=> a takeaway, I strongly suggest to change

buildPlugin(platforms: ['linux','windows'])

by

buildPlugin(
  useContainerAgent: true,
  configurations: [
    [platform: 'linux', jdk: 11],
    [platform: 'windows', jdk: 11],
])

Copy link
Contributor

Choose a reason for hiding this comment

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

When you say linux, do you mean x86 or arm64? With or without Docker Engine?

I mean I do not care :) (currently as it was restricted - linux is ambiguos and the useage of it in this library has not helped plugin authors)..

But in reality it may depend on if this is actually allowing a label, or a label expression. If it would be a label expression then a developer could say linux && docker && x64
Would that pick up highmen - as is very likely - but we should be able to make highmen (and other expensive options) only available if a specific label is actually given. This may require a plugin (not sure off hand) but would seem like a useful thing to have to the wider community.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, our analysis matches yours! That would be the road to a (breaking) change in the provided labels on ci.jenkins.io.

The behavior in label matching you describe is the reason why, in 2021, for 2 months, we used ARM VM agents for a lot of builds without even knowing it, because linux was resolved by both ARM and x86 machines.
=> As soon as Jenkins find an agent template matching the labels, it uses it.

We are thinking about a policy with looooong tags such as <os>-<cpu>-<qualifier> but still note sure where to put the cursor with or without too much details as we have different dimensions but it might not be needed (or event wanted) to provide all of them:

  • OS (Linux Ubuntu, Windows Core Server)
  • OS version (22.04 for Ubuntu, 2019/2022 for Windows Core Server)
  • size (such as highmem)
  • Compute type: VM, container
  • Tools (docker, maven, jdk17)
  • Feature: jdk-next (for edge non stable JDK), experiemental, etc.
  • Cloud/Location (aws, azure, etc.)
  • Infrastructure (such as cluster name or region name: doks, etc.)

We though about the platform labeler plugin, but haven't look or tried closely. Any idea or tip is welcome.

label = platform
}
}

Expand Down