Skip to content

[WiP] - Change towards hosting modules on ci.jenkins.io#6

Closed
oleg-nenashev wants to merge 3 commits into
jenkins-infra:masterfrom
oleg-nenashev:modules-build-flow
Closed

[WiP] - Change towards hosting modules on ci.jenkins.io#6
oleg-nenashev wants to merge 3 commits into
jenkins-infra:masterfrom
oleg-nenashev:modules-build-flow

Conversation

@oleg-nenashev
Copy link
Copy Markdown
Contributor

I work on the new release of the Windows Agent Installer Module, which currently has no active CI. So I would like to have a build flow defined by this lib. Downstream PR with Jenkinsfile: jenkinsci/windows-slave-installer-module#7

Changes:

  • - Add the new buildModule() command. Now it is just a wrapper on the top of buildPlugin(), but it may change in the future (e.g. integration tests with the latest Jenkins core, etc.).
  • - Add the allowEmptyTestResults option to both buildPlugin() and buildModule()

@reviewbybees, esp. @rtyler and @jglick

@oleg-nenashev oleg-nenashev changed the title Change towards hosting modules on ci.jenksins.io Change towards hosting modules on ci.jenkins.io Feb 28, 2017
@ghost
Copy link
Copy Markdown

ghost commented Feb 28, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@jglick
Copy link
Copy Markdown
Contributor

jglick commented Feb 28, 2017

integration tests with the latest Jenkins core

That is already supported with buildPlugin.

Add the allowEmptyTestResults option

This seems like a bad idea. You should always have InjectedTest at least.

Comment thread README.adoc Outdated
* `jenkinsVersions`: (default: `[null]`) - a matrix of Jenkins baseline versions to build/test against in parallel (null means default,
only available for Maven projects)
* `allowEmptyTestResults` (default: `false`) - Do not fail the build if the test results are missing.
It may be used for components, which have no tests within the repo
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Delete comma. You are saying something grammatically valid but quite different than what I think you meant: “components have no tests, so you may use this flag for them” → “if you have a component which has no tests, then you may use this flag for it” (subtle point of English punctuation; I know at least Czech lacks this rule)

Comment thread vars/buildModule.groovy Outdated

def call(Map params = [:]) {
buildPlugin(params)
} No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add trailing newline

@oleg-nenashev
Copy link
Copy Markdown
Contributor Author

That is already supported with buildPlugin.

Maybe. But having a separate command is definitely reasonable for the future use

This seems like a bad idea. You should always have InjectedTest at least.

Not for the Jenkins module

@jglick
Copy link
Copy Markdown
Contributor

jglick commented Feb 28, 2017

Not for the Jenkins module

Well I disagree with that, but will leave it to the downstream PR.

@rtyler
Copy link
Copy Markdown
Member

rtyler commented Feb 28, 2017

Maybe. But having a separate command is definitely reasonable for the future use

@oleg-nenashev would you mind adding some commentary into the buildModule.groovy itself explaining what potential future use-cases there might be? I don't yet understand why a redundant API is necessary.

@oleg-nenashev
Copy link
Copy Markdown
Contributor Author

@rtyler Let's put in on hold for a while. I really need to come-up with a realistic use-case for the Jenkins module lifecycle, but my previous ideas seem to be not really relevant since Jenkins modules are too close to plugins now.

@oleg-nenashev oleg-nenashev changed the title Change towards hosting modules on ci.jenkins.io [WiP] - Change towards hosting modules on ci.jenkins.io Feb 28, 2017
@rtyler
Copy link
Copy Markdown
Member

rtyler commented May 9, 2017

I'm going to close this out until we can figure out whether there's a strong use-case here. Feel free to re-open it if something comes up.

@rtyler rtyler closed this May 9, 2017
@oleg-nenashev oleg-nenashev mentioned this pull request Aug 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants