New CI pipeline #1392

tyvision · 2018-05-29T10:39:12Z

Description of the Change

This PR is created to finally make our CI satisfy proposed requirements by our developers team.

1 Major changes

1.1 Pipeline stages

Since now, the Pipeline consists of 6 stages. You can see the ascii-art pipeline version in the Jenkinsfile

Stage	Description
Pre-build	Cancels previous build job (except if current branch is `develop`)
Build	Build step for all platforms (currently `ARMv7`, `ARMv8`, `x86_64`, `MacOS` are present)
Pre-coverage	Launch coverage target before running tests
Tests	Run required tests for specified platforms
Post-Coverage	Run coverage after tests step
Build rest	Build (if required): 1) documentation 2) bindings 3) release version of `iroha`

This division is made to improve visibility of the CI build process.
Stage separation is implemented in Jenkinsfile. All stage methods are implemented in .jenkinsci/debug-build.groovy, .jenkinsci/release-build.groovy, .jenkinsci/mac-debug-build.groovy, .jenkinsci/mac-release-build.groovy (note that MacOS build goes on the physical machine, not in the docker container, that's why there's another file for MacOS platform)

1.2 On-demand Jenkins agents

Since now x86_64 agents are moved to AWS EC2. There are 4 different labels added to the pipeline (note that it could be easily modified in Jenkins configuration):

x86_64_aws_build -- general purpose instance with 8 CPU, 32 Gb RAM (x86_64 builds in 8 threads)
x86_64_aws_test -- general purpose instance with 1 CPU, 2 Gb RAM for running tests
x86_64_aws_cov -- general purpose instance with 1 CPU, 2 Gb RAM for running coverage
x86_64_aws_sonar -- general purpose instance with 1 CPU, 2 Gb RAM for running sonarscanner

Now, the amount of x86_64 agents will be float, as their amount is managed by Jenkins.

Note: With migration to Amazon AWS EC2 on-demand agents the logic of some methods has been changed. The text below describes this change.

On-demand agent start-up process
Before freshly created and launched new AWS instance becomes the Jenkins agent, init script is executed. It mounts 4 folders via NFS share (AWS EFS - Amazon Elastic Filesystem).

Jenkins workspace (/var/jenkins)
cmake build cache (/opt/.ccache)
cmake release build cache (/opt/.ccache-release)
docker images storage (/tmp/docker)

On-demand agent pipeline lifecycle
In case the build occures on the x86_64* agent, the created docker image (stage Build) is saved to AWS EFS (Amazon Elastic Filesystem) using docker save -o command with file name /tmp/docker/<branch_name_sha1>. (see file .jenkinsci/docker-pull-or-build.groovy, method dockerPullOrUpdate()). When going to the next stage, new x86_64* agent is created, docker image is loaded from the file /tmp/docker/<branch_name_sha1> using docker load command.

After the stage finishes, the docker container is stopped and removed, results are saved to the Jenkins workspace (and to the AWS EFS respectively), thus, none of the data is lost.

1.3 Merge availability

To make CI available to perform automatic merge process the list of features should be implemented:

check merge requirements compliance
merge strategy

Merge requirements set in hyperledger/iroha repository:

at least 2 APPROVED reviews
CI checks are passed (armv7, armv8 are turned off in case of MergePR is chosen)
all commits are sign-off

Merge strategy regarding the branches:

trunk + develop -- squash & merge
master -- merge

All checks are made using Github API. To see the implementation, go to .jenkinsci/github-api.groovy file.

To merge, developer should go to Jenkins -> iroha -> iroha-hyperledger -> Pull Requests -> PR # -> Build with Parameters and set checkpoint for Merge_PR.

1.4 CI notifications

Build result notifications were added. Table below describes the notification scenario:

Scenario	Behavior
Branch commit	sends e-mail to git commit author
Open or Commit Pull Request	posts comment with PR reviewers mention on the PR page
Merge to `trunk`	sends e-mail to git commit author
Merge to `develop`	sends e-mail to git commit author + Andrei + Fyodor
Merge to `master`	sends e-mail to iroha-maintainers team

Notification e-mail contains compressed log in attachmens + build status.
see .jenkinsci/notifications.groovy,Jenkinsfile (cleanup post-step at the end of the file)

1.5 Tests launch separation

This feature is implemented as a method that returns regex string (file .jenkinsci/test-launcher.groovy)
This method is called in the Jenkinsfile stage Tests and its return string is passed as a parameter to the doTestStep() method. More detailed description of test types execution according to the build strategy is provided in the next section (1.6)

1.6 Default order of execution

Table contains requirements for order of pipeline execution.

Scenario	Build Behavior	Coverage	Tests	Packages	Docs	Bindings
Branch commit	x86_64 gcc-5.4 build	by request	`module` tests	by request	by request	by request
Open Pull Request	`x86_64 gcc-5.4`, `MacOS` build	for `x86_64`	`module`	by request	by request	by request
Commit Pull Request	`x86_64 gcc-5.4`	by request	`module`	by request	by request	by request
Merge to `trunk`	all platforms	by request	`module` for all platforms. for `x86_64 gcc-5.4` run `integration`, `system`, `cmake`, `regression` tests	by request	by request	by request
Merge to `develop`	all platforms	by request	`module` for all platforms. for `x86_64 gcc-5.4` run `integration`, `system`, `cmake`, `regression` tests	docker dev	all	all
Merge to `master`	all platforms	`x86_64 gcc-5.4`	`module` for all platforms. for `x86_64 gcc-5.4` run `integration`, `system`, `cmake`, `regression` tests	all	all	all
Nightly	all platforms	`x86_64 gcc-5.4`	for all platforms run `integration`, `system`, `cmake`, `regression` tests. for `x86_64 gcc-5.4` run all tests	`.deb`, `.tar`	none	all

2 Minor changes

Fixed absence of --output-on-failure flag on MacOS tests.
Modified post-step for whole CI allowed to fix execution logic
Fixed clean-up procedures:
- separated linuxPostStep() method from jenkinsci/linux-post-step.groovy into postStep() and cleanUp() methods. postStep() method is authoritative for uploading artifacts after release build, when cleanUp() removes folders created during builds
- fixed the call cases for the doDockerCleanup(). Now this method been called only from the pipeline's cleanup post-step, at armv7, armv8 agents.
- fixed the stale docker images cleanup (IR-1293, IR-1451)

Benefits

reduced build time (~15-20 mins for branch commit, except branch commit with changes dockerfile, it takes additional 30-50 minutes)
automatic merge by CI
defined workflow that covers all scenario

Usage Examples or Tests [optional]

bakhtin · 2018-05-29T11:53:12Z

.jenkinsci/debug-build.groovy

  // This is a known bug. See https://issues.jenkins-ci.org/browse/JENKINS-41929
-  if (!parallelism) {
+  if (parallelism == null) {


Please, revert to !parallelism as it was fixed in previous commits. You have not merged properly, I suppose

Yes, I've not rebased properly. It will be fixed in the next commit

bakhtin · 2018-05-29T12:20:02Z

.jenkinsci/docker-pull-or-build.groovy

@@ -19,7 +19,13 @@ def buildOptionsString(options) {

 def dockerPullOrUpdate(imageName, currentDockerfileURL, previousDockerfileURL, referenceDockerfileURL, buildOptions=null) {
  buildOptions = buildOptionsString(buildOptions)
+  // GIT_PREVIOUS_COMMIT is null for first PR build
+  if (!previousDockerfileURL) {


This condition always returns true

bakhtin · 2018-05-29T12:40:28Z

.jenkinsci/debug-build.groovy

@@ -53,62 +112,49 @@ def doDebugBuild(coverageEnabled=false) {
      + " -e IROHA_POSTGRES_USER=${env.IROHA_POSTGRES_USER}"
      + " -e IROHA_POSTGRES_PASSWORD=${env.IROHA_POSTGRES_PASSWORD}"
      + " --network=${env.IROHA_NETWORK}"
-      + " -v /var/jenkins/ccache:${CCACHE_DIR}"
+      + " -v ${CCACHE_DIR}:${CCACHE_DIR}"


CCache here has no effect. It does not speed up tests

bakhtin · 2018-05-29T12:41:03Z

.jenkinsci/debug-build.groovy

@@ -53,62 +112,49 @@ def doDebugBuild(coverageEnabled=false) {
      + " -e IROHA_POSTGRES_USER=${env.IROHA_POSTGRES_USER}"
      + " -e IROHA_POSTGRES_PASSWORD=${env.IROHA_POSTGRES_PASSWORD}"
      + " --network=${env.IROHA_NETWORK}"
-      + " -v /var/jenkins/ccache:${CCACHE_DIR}"
+      + " -v ${CCACHE_DIR}:${CCACHE_DIR}"
      + " -v /tmp/${GIT_COMMIT}-${BUILD_NUMBER}:/tmp/${GIT_COMMIT}") {


No need to mount artifacts folder for tests stage

bakhtin · 2018-05-29T13:30:44Z

.jenkinsci/debug-build.groovy

+  if ( env.NODE_NAME.contains('x86_64') ) {
+    sh "docker load -i ${JENKINS_DOCKER_IMAGE_DIR}/${dockerImageFile}"
+  }
+  def iC = docker.image("${dockerAgentImage}")


This variable is only assigned if NODE_NAME contains x86_64. Thus, will not work on other agents. Review also other functions

bakhtin · 2018-05-30T10:35:46Z

Jenkinsfile

+              if ( params.BUILD_TYPE == 'Debug' || params.Merge_PR ) {
+                def debugBuild = load ".jenkinsci/debug-build.groovy"
+                def coverage = load ".jenkinsci/selected-branches-coverage.groovy"
+                if ( params.Merge_PR ) { debugBuild.doDebugBuild(true) }


Would coverage be collected twice: first on PR open and then on actual merge?

for master branch --> yes, it is according to the docs. Conditions are fixed in the next commit

bakhtin · 2018-05-30T10:49:40Z

Jenkinsfile

+            anyOf {
+              allOf {
+                expression { return env.CHANGE_ID != null }
+                not { expression { return env.GIT_PREVIOUS_COMMIT != null } }


Double negation can be replaced with
{ expression { return ! env.GIT_PREVIOUS_COMMIT }
Also, seems that GIT_PREVIOUS_COMMIT variable value is not set on a newer versions of Jenkins' Git plugin on PR open. See Jenkins docs: older version vs newer version

bakhtin · 2018-05-30T11:39:06Z

Jenkinsfile

      when {
-        expression { params.BUILD_TYPE == 'Release' }
+        beforeAgent true
+        anyOf {


can be omitted

bakhtin · 2018-05-30T11:52:48Z

Jenkinsfile

+          steps {
+            script {
+              def cov_platform = ''
+              if (params.Linux) {


Maybe implement this platform selection as a separate function?

implemented in the .jenkinsci/choose-platform.groovy

bakhtin · 2018-05-30T11:54:28Z

Jenkinsfile

+              allOf {
+                expression { return env.CHANGE_ID != null}
+                expression { return env.GIT_PREVIOUS_COMMIT != null }
+                expression { return params.Merge_PR }


expression { return env.CHANGE_TARGET ==~ /(master|develop|trunk)/ } is not required here, why?

actually we can remove the whole section when in parallel stages in Post-coverage as it is needless after when in Post-coverage

muratovv · 2018-06-04T10:58:39Z

.jenkinsci/debug-build.groovy

  // params are always null unless job is started
  // this is the case for the FIRST build only.
-  // So just set this to same value as default.
+  // So just set this to same value as default. 


Redundant space.

muratovv · 2018-06-04T11:15:25Z

.jenkinsci/choose-platform.groovy

+
+def choosePlatform() {
+	if (params.Merge_PR) {
+		return 'x86_64_aws_cov'


Instead of string literals better to use enums. This change improves readability and reduces type errors based on the type system.

bakhtin · 2018-06-01T12:01:06Z

.jenkinsci/choose-platform.groovy

+#!/usr/bin/env groovy
+
+def choosePlatform() {
+	if (params.Merge_PR) {


Fix indentation

@bakhtin fixed

bakhtin · 2018-06-04T11:05:18Z

.jenkinsci/docker-pull-or-build.groovy

@@ -19,65 +19,59 @@ def buildOptionsString(options) {

 def dockerPullOrUpdate(imageName, currentDockerfileURL, previousDockerfileURL, referenceDockerfileURL, buildOptions=null) {
  buildOptions = buildOptionsString(buildOptions)
+  def uploadExitCode = 0
  // GIT_PREVIOUS_COMMIT is null for first PR build


I do not think you can simply remove this block. You should check whether previousDockerfileURL is null. Otherwise, you end up querying URL with literal null string in it.

@bakhtin I think we can as previousDockerfileURL passed to the dockerPullOrBuild() method is built from pCommit variable, which cannot be null, otherwise it's out of scope of method dockerPullOrBuild() and should be fixed in the .jenkinsci/previous-commit.groovy previousCommitOrCurrent() method

bakhtin · 2018-06-04T11:30:20Z

.jenkinsci/docker-pull-or-build.groovy

+        sh "echo 'Reference image ${DOCKER_REGISTRY_BASENAME}:${imageName} doesn't exist on the EFS"
+      }
+    }
+    def pullExitCode = sh(script: "docker pull ${DOCKER_REGISTRY_BASENAME}:${imageName}", returnStatus: true)


docker.image in the last else block makes implicit pull. No need to call it earlier explicitly.

@bakhtin no, it doesn't actually. No other docker pull commands are used inside dockerPullOrBuild() method

bakhtin · 2018-06-04T11:34:10Z

.jenkinsci/linux-post-step.groovy

-      }
-      cleanWs()
-    }
+// def linuxPostStep() {


Remove commented out lines if they're not needed anymore

@bakhtin removed in the next commit

muratovv · 2018-06-04T12:07:49Z

.jenkinsci/github-api.groovy

@@ -0,0 +1,113 @@
+#!/usr/bin/env groovy
+
+def mergePullRequest() {


Pls, add some description of methods because now it is not so obvious what really this method does.

@muratovv added

muratovv · 2018-06-04T12:09:52Z

.jenkinsci/github-api.groovy

+	}
+}
+
+def checkMergeAcceptance() {


Also, pls, add a description of the current method and for methods below.

@muratovv added

muratovv · 2018-06-04T12:13:03Z

.jenkinsci/github-api.groovy

+		jsonResponseReview = slurper.parseText(jsonResponseReview)
+		if (jsonResponseReview.size() > 0) {
+			jsonResponseReview.each {
+				if ("${it.state}" == "APPROVED") {


Also, we must forbid merge when someone "request changes" for the pull request.
Generally, we require 2 approves and zero "request changes" for pull requests.

@muratovv good point, I will add this feature at the next commit

muratovv · 2018-06-04T12:15:00Z

.jenkinsci/github-api.groovy

+
+def getMergeMethod() {
+	if ( env.CHANGE_TARGET == 'master') {
+		return "merge"


Also, better to use enums for performing business logic instead of string literals.

@muratovv fixed

muratovv · 2018-06-04T12:20:56Z

.jenkinsci/github-api.groovy

+		def slurper = new groovy.json.JsonSlurperClassic()
+		def jsonResponseComment = sh(script: """
+			curl -H "Authorization: token ${sorabot}" \
+			-H "Accept: application/vnd.github.v3+json" \


Maybe better to make a separate function that will call Github API?

@muratovv no, as each API call is different:

GET, PUT, POST query

with or w/o token

muratovv · 2018-06-04T12:24:41Z

.jenkinsci/linux-post-step.groovy

+//   }
+// }
+
+def postStep() {


Add documentation for methods here.

muratovv · 2018-06-04T12:26:05Z

.jenkinsci/mac-debug-build.groovy

+def doDebugBuild(coverageEnabled=false) {
+  def parallelism = params.PARALLELISM
+  if (!parallelism) {
+    parallelism = 4


This code looks suspicious. What is the point to make 4 threads in the non-parallel build?
Also, better to pass a number of threads in the function and create another one for calculating a number.

@muratovv now unique method in the file.jenkinsci/choose-platform.groovy called setParallelism is created. It is a silver bullet for the whole pipeline execution branches

muratovv · 2018-06-04T12:30:26Z

.jenkinsci/mac-debug-build.groovy

+    }
+}
+
+def doPostCoverageCoberturaStep() {


What is the point to duplicate code here and in debug-build file?

in debug-build.groovy the post-coverage is launched inside a docker-container. in mac-debug-build.groovy the post-coverage is launched on the host OS with the same logic.

Thus, in debug-build.groovy I can call doPostCoverageCoberturaStep from mac-debug-build.groovy to keep DRY. Will be changed in the next commit

muratovv · 2018-06-04T12:30:43Z

.jenkinsci/mac-release-build.groovy

@@ -1,6 +1,10 @@
 #!/usr/bin/env groovy

 def doReleaseBuild(coverageEnabled=false) {
+  def parallelism = params.PARALLELISM
+  if (!parallelism) {
+    parallelism = 4


Same about threads.

@muratovv fixed

muratovv · 2018-06-04T12:33:46Z

.jenkinsci/test-launcher.groovy

+def chooseTestType() {
+	if (params.Merge_PR) {
+		if (env.NODE_NAME.contains('x86_64')) {
+			return "(module|integration|system|cmake|regression)*"	


Also, better to write direct types in enums and operate on it.

@muratovv fixed in the next commit

bakhtin · 2018-06-11T06:22:30Z

.jenkinsci/test-launcher.groovy

+def printRange(start, end) {
+  def output = ""
+  for (type in start..end) {
+    output = [output, (type.name() != start.toString() ? "|" : ""), type.name()].join('')


join() already implements the logic of not putting delimeter after the last element in the list.

@bakhtin join() cannot work with enum. So we can either do as I proposed in the commit, either transform enum to string collection/tuple and use join() which is even more complex

bakhtin · 2018-06-11T06:27:33Z

.jenkinsci/test-launcher.groovy

 		}
 	}
-	return "module*"
+	// just choose module tests
+	return printRange(TestTypes.module, TestTypes.module)


A typo? Should be just return printRange(TestTypes.module) ?

muratovv · 2018-06-13T13:10:29Z

.jenkinsci/docker-cleanup.groovy

+    # Check whether the image is the last-standing man
+    # i.e., no other tags exist for this image
+    docker rmi \$(docker images --no-trunc --format '{{.Repository}}:{{.Tag}}\\t{{.ID}}' | grep \$(docker images --no-trunc --format '{{.ID}}' ${iC.id}) | head -n -1 | cut -f 1) || true
+    sleep 5


What is the point of sleep? Can we rework it with the synchronous check of rmi?
Maybe rework it in separate PR, if it is, pls add a task for that.

@muratovv this is a required delay for docker engine to proceed the image removal. Actually, it's a kind of legacy from previous pipeline and left untouched

muratovv · 2018-06-13T13:10:50Z

.jenkinsci/enums.groovy

@@ -0,0 +1,10 @@
+enum TestTypes {
+	module, integration, system, cmake, regression, benchmark, framework


Add explanation of types here.

muratovv · 2018-06-13T13:11:05Z

.jenkinsci/enums.groovy

+}
+
+enum CoveragePlatforms {
+  x86_64_aws_cov, mac, armv8, armv7


add explanation of types here.

@muratovv done

muratovv · 2018-06-13T13:11:52Z

.jenkinsci/github-api.groovy

@@ -1,5 +1,16 @@
 #!/usr/bin/env groovy

+enum GithubPRStatus {


Add a documentation for this enum and enums below.

@muratovv done

muratovv · 2018-06-13T13:22:41Z

.jenkinsci/choose-platform.groovy

-	if (params.Merge_PR) {
-		return 'x86_64_aws_cov'
+  if (params.Merge_PR) {
+		return CoveragePlatforms.x86_64_aws_cov.toString()


What is the point of type erasure (return a string) instead of returning an enum value?

@muratovv agent label is a string type, thus, we need to return string

muratovv · 2018-06-13T13:26:43Z

.jenkinsci/github-api.groovy

@@ -84,14 +103,15 @@ def getPullRequestReviewers() {
 	jsonResponseReview = slurper.parseText(jsonResponseReview)
 	if (jsonResponseReview.size() > 0) {
 		jsonResponseReview.each {
-			if ("${it.state}" == "APPROVED") {
+			if ("${it.state}" == GithubPRStatus.APPROVED.toString() || "${it.state}" == GithubPRStatus.CHANGES_REQUESTED.toString()) {


Better to clean up like this:

if( "${it.state}" in [GithubPRStatus.APPROVED.toString(), GithubPRStatus.CHANGES_REQUESTED.toString()])

lebdron

I would love to see the changes based on this review before another round of review, because it should reduce the code.

lebdron · 2018-06-18T18:07:46Z

.jenkinsci/choose-platform.groovy

+    return CoveragePlatforms.x86_64_aws_cov.toString()
+  }
+  if (!params.Linux && params.MacOS) {
+    return CoveragePlatforms.mac.toString()


The requirement is to collect coverage only on gcc/Linux, so maybe mac, armv8, and armv7 can be removed?

@lebdron according to the requirements, this file is not needed anymore. It's removed in the next commit

lebdron · 2018-06-18T18:09:22Z

.jenkinsci/debug-build.groovy

+  }
+  def iC = docker.image("${DOCKER_AGENT_IMAGE}")
+  iC.inside() {
+    def step = load ".jenkinsci/mac-debug-build.groovy"


Same for coverage, I don't think any other platform besides gcc/Linux is required.

lebdron · 2018-06-18T18:10:12Z

.jenkinsci/enums.groovy

+
+// list of agent labels (build platforms) to count coverage (the choice depends on the build parameters)
+enum CoveragePlatforms {
+  x86_64_aws_cov, mac, armv8, armv7


Same for coverage, just leave x86_64_aws_cov ?

@lebdron removed. good point

lebdron · 2018-06-18T18:10:45Z

.jenkinsci/mac-debug-build.groovy

@@ -0,0 +1,70 @@
+def doDebugBuild(coverageEnabled=false) {


Flag should not be required ^^

@lebdron also fixed in the next commit

lebdron · 2018-06-18T18:16:09Z

.jenkinsci/mac-debug-build.groovy

+}
+
+def doPostCoverageSonarStep() {
+  sh "cmake --build build --target cppcheck"


Same for cppcheck, the code analysis output should be the same if sent from mac os or Linux, so it should be only run in gcc/Linux.

All methods related to coverage are removed from the mac-debug-build.groovy file.

lebdron · 2018-06-18T18:18:42Z

Jenkinsfile

+  booleanParam(defaultValue: false, description: '', name: 'ARMv7'),
+  booleanParam(defaultValue: false, description: '', name: 'ARMv8'),
+  booleanParam(defaultValue: false, description: '', name: 'MacOS'),
+  booleanParam(defaultValue: false, description: 'Whether it is a triggered build', name: 'Nightly'),


I would remove "whether" from titles, because the switches themselves imply this word, so it does not add any information.

lebdron · 2018-06-18T18:20:18Z

Jenkinsfile

+              if ( params.BUILD_TYPE == 'Debug' || params.Merge_PR ) {
+                def debugBuild = load ".jenkinsci/debug-build.groovy"
+                def coverage = load ".jenkinsci/selected-branches-coverage.groovy"
+                debugBuild.doDebugBuild( (!params.Linux && !params.MacOS || !params.Merge_PR) ? coverage.selectedBranchesCoverage() : false )


This conditional could be removed because of reasons above, same for calls for mac, armv7.

Thanks, now we can get rid of these complex conditions

bakhtin

GBTPR (God bless this PR)

bakhtin · 2018-06-19T11:20:16Z

.jenkinsci/docker-cleanup.groovy

-    """
+// cleanup docker images which weren't used for more that 20 days and image for this PR in case of successful PR
+def doStaleDockerImagesCleanup() {
+  sh "find ${JENKINS_DOCKER_IMAGE_DIR} -type f -mtime +20 -exec rm -f {}"


This fails with an error on Ubuntu 16.04. Did you check this really works? I think it should end with \; after curly braces.

@bakhtin Thanks for the report, I've missed \;. I've just checked it on the real files (AWS EFS) and it WORKS!

lebdron · 2018-06-19T15:58:30Z

Jenkinsfile

      agent { label 'master' }
      steps {
        script {
+          load ".jenkinsci/enums.groovy"


Output of load is not used, consider removing.

@lebdron no, this is required to load static variable of enum. Output shouldn't be used here

lebdron · 2018-06-19T16:00:45Z

Jenkinsfile

-            expression { return params.armv7_linux }
+            anyOf {
+              expression { return params.ARMv7 }
+              // allOf {


Please add a TODO to fix issues and uncomment, with Jira task id. Same for ARMv8 below.

lebdron · 2018-06-19T16:04:44Z

Jenkinsfile

-                cobertura autoUpdateHealth: false, autoUpdateStability: false, coberturaReportFile: '**/build/reports/coverage.xml', conditionalCoverageTargets: '75, 50, 0', failUnhealthy: false, failUnstable: false, lineCoverageTargets: '75, 50, 0', maxNumberOfBuilds: 50, methodCoverageTargets: '75, 50, 0', onlyStable: false, zoomCoverageChart: false
+              if ( params.BUILD_TYPE == 'Debug' || params.Merge_PR ) {
+                def macDebugBuild = load ".jenkinsci/mac-debug-build.groovy"
+                def coverage = load ".jenkinsci/selected-branches-coverage.groovy"


coverage is unused, please remove.

lebdron · 2018-06-19T16:06:15Z

.jenkinsci/linux-post-step.groovy

+}
+
+// upload artifacts in release builds (for mac)
+def macPostStep() {


File name is confusing (linux, but also contains mac methods), maybe rename the file or move the methods?

@lebdron I've fixed the filename

lebdron · 2018-06-19T16:09:38Z

Jenkinsfile

+                  }
+                  finally {
+                    sh "rm -rf /tmp/${env.GIT_COMMIT}"
+                    // cleanWs()


Please remove if it is not required.

@lebdron it was commented out by an accident

lebdron · 2018-06-19T16:29:07Z

.jenkinsci/selected-branches-coverage.groovy

-	if (PRCoverage) {
-		return env.GIT_LOCAL_BRANCH in branches || env.CHANGE_ID != null
+	def pCommit = load ".jenkinsci/previous-commit.groovy"
+	def prevCommit = pCommit.previousCommitOrCurrent()


PREVIOUS_COMMIT environment variable defined in Jenkinsfile can be used intstead.

lebdron · 2018-06-19T16:35:09Z

Jenkinsfile

-              else {
+              if ( params.BUILD_TYPE == 'Debug' || params.Merge_PR ) {
+                def debugBuild = load ".jenkinsci/debug-build.groovy"
+                def coverage = load ".jenkinsci/selected-branches-coverage.groovy"


Please remove because it is unused.

lebdron · 2018-06-19T16:47:26Z

Jenkinsfile

+            expression { return !env.GIT_PREVIOUS_COMMIT }
+          }
+          allOf {
+            expression { return env.CHANGE_ID != null }


After discussion with @tyvision we decided to move these complex conditions to separate variables as SOME_VAR = [[ env.CHANGE_ID != null && env.CHANGE_TARGET ==~ /(master|develop|trunk)/ ]], and then use them here. These variables will have documentation where they are defined, and the logic will not be duplicated in different stages.

muratovv

I am very scared about this PR, but I don't have critical issues. I hope we will not revert this pr in develop branch.

muratovv · 2018-06-27T10:02:27Z

.jenkinsci/set-parallelism.groovy

+  if (!defaultParameter) {
+    return 4
+  }
+  if (env.NODE_NAME.contains('arm7')) {


Looks like here is possible to use the enum with machine types

muratovv · 2018-06-27T10:05:13Z

.jenkinsci/test-launcher.groovy

+// return tests list regex that will be launched by ctest
+def chooseTestType() {
+	if (params.merge_pr) {
+		if (env.NODE_NAME.contains('x86_64')) {


same about enums

@muratovv My concern is to leave this update for the next PR on CI (not as major as this one)

muratovv · 2018-06-27T10:11:57Z

Jenkinsfile

-            expression { return params.armv7_linux }
+            anyOf {
+              expression { return params.armv7_linux }
+              // expression { return MERGE_CONDITIONS_SATISFIED == "true" }


What is the point of this line?

It is disabled because current armv7 builds fail due to insufficient memory. Builds should be possible with cross-compilation, so this can serve as a reminder.

Signed-off-by: tyvision <[email protected]>

Signed-off-by: Anatoly <[email protected]>

Signed-off-by: tyvision <[email protected]>

Signed-off-by: Anatoly <[email protected]>

Signed-off-by: tyvision <[email protected]>

tyvision requested review from muratovv, bakhtin and lebdron May 29, 2018 10:39

bakhtin suggested changes May 30, 2018

View reviewed changes

l4l added needs-review pr awaits review from maintainers infrastructure anything related to continious integration, building infrastructure, or cmake labels May 30, 2018

tyvision force-pushed the feature/new-ci-pipeline branch 2 times, most recently from 3e808bd to 429fd96 Compare June 1, 2018 14:03

muratovv reviewed Jun 4, 2018

View reviewed changes

bakhtin reviewed Jun 4, 2018

View reviewed changes

muratovv reviewed Jun 4, 2018

View reviewed changes

muratovv suggested changes Jun 4, 2018

View reviewed changes

bakhtin reviewed Jun 11, 2018

View reviewed changes

muratovv reviewed Jun 13, 2018

View reviewed changes

muratovv suggested changes Jun 13, 2018

View reviewed changes

tyvision force-pushed the feature/new-ci-pipeline branch from e73340e to 5ede358 Compare June 16, 2018 09:06

lebdron suggested changes Jun 18, 2018

View reviewed changes

bakhtin approved these changes Jun 19, 2018

View reviewed changes

lebdron reviewed Jun 19, 2018

View reviewed changes

tyvision force-pushed the feature/new-ci-pipeline branch 2 times, most recently from 05c1810 to d4a43e4 Compare June 21, 2018 17:15

muratovv approved these changes Jun 27, 2018

View reviewed changes

lebdron approved these changes Jun 27, 2018

View reviewed changes

tyvision added 2 commits June 28, 2018 11:49

prepare major-change Pull Request

0b72b5d

Signed-off-by: tyvision <[email protected]>

fix review results

ffdf15c

Signed-off-by: tyvision <[email protected]>

tyvision force-pushed the feature/new-ci-pipeline branch from e4acb38 to ffdf15c Compare June 28, 2018 08:49

tyvision added 2 commits June 28, 2018 13:53

fix docs and bindings in pipeline

807631c

Signed-off-by: tyvision <[email protected]>

fix bindings

58d30eb

Signed-off-by: tyvision <[email protected]>

tyvision added 3 commits June 28, 2018 17:00

fix again

33c9f63

Signed-off-by: tyvision <[email protected]>

fixed bindings

3c8ae28

Signed-off-by: tyvision <[email protected]>

fix bindings

558ae10

Signed-off-by: tyvision <[email protected]>

l4l removed the needs-review pr awaits review from maintainers label Jun 29, 2018

tyvision added 2 commits June 29, 2018 13:47

fixed workspace

731896c

Signed-off-by: tyvision <[email protected]>

new ci is ready to merge

e856ffd

Signed-off-by: tyvision <[email protected]>

sorabot merged commit 30cc22e into develop Jun 29, 2018

tyvision added a commit that referenced this pull request Jun 29, 2018

hotfix (#1392)

f6a6c4d

Signed-off-by: tyvision <[email protected]>

Stayer mentioned this pull request Jun 29, 2018

Incorrect CI build parameters #1519

Closed

tyvision added a commit that referenced this pull request Jun 29, 2018

hotfix linux-release upload (#1392)

a7fb62c

Signed-off-by: tyvision <[email protected]>

tyvision added a commit that referenced this pull request Jun 29, 2018

hotfixes to #1392

d635c34

Signed-off-by: tyvision <[email protected]>

tyvision added a commit that referenced this pull request Jun 29, 2018

hotfixes to #1392

e0e31ff

Signed-off-by: tyvision <[email protected]>

tyvision added a commit that referenced this pull request Jun 29, 2018

hotfixes to #1392

528d4cf

Signed-off-by: tyvision <[email protected]>

tyvision added a commit that referenced this pull request Jun 29, 2018

hotfixes to #1392

68b5490

Signed-off-by: tyvision <[email protected]>

tyvision deleted the feature/new-ci-pipeline branch June 30, 2018 10:22

tyvision added a commit that referenced this pull request Jul 3, 2018

Revert "New CI pipeline (#1392)"

ad3dbe6

Signed-off-by: tyvision <[email protected]>

tyvision mentioned this pull request Jul 3, 2018

Revert New CI Pipeline #1392 #1533

Merged

tyvision added a commit that referenced this pull request Jul 3, 2018

Revert "New CI pipeline (#1392)" (#1533)

f309501

Signed-off-by: tyvision <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

New CI pipeline (#1392)

9e32869

l4l pushed a commit that referenced this pull request Jul 25, 2018

hotfixes to #1392

4bad1d9

Signed-off-by: tyvision <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Revert "New CI pipeline (#1392)" (#1533)

e0f2eca

Signed-off-by: tyvision <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

New CI pipeline (#1392)

e51754c

l4l pushed a commit that referenced this pull request Jul 25, 2018

hotfixes to #1392

cd1fe22

Signed-off-by: tyvision <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Revert "New CI pipeline (#1392)" (#1533)

5302bf9

Signed-off-by: tyvision <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

New CI pipeline (#1392)

1037191

Signed-off-by: Anatoly <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

hotfixes to #1392

f8537eb

Signed-off-by: tyvision <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Revert "New CI pipeline (#1392)" (#1533)

2f14929

Signed-off-by: tyvision <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Revert "New CI pipeline (#1392)" (#1533)

58da20d

Signed-off-by: tyvision <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Revert "New CI pipeline (#1392)" (#1533)

d87c204

Signed-off-by: Anatoly <[email protected]>

Stayer pushed a commit that referenced this pull request Jul 27, 2018

hotfix (#1392)

df7ec55

Signed-off-by: tyvision <[email protected]>

		@@ -0,0 +1,113 @@
		#!/usr/bin/env groovy

		def mergePullRequest() {

		@@ -0,0 +1,10 @@
		enum TestTypes {
		module, integration, system, cmake, regression, benchmark, framework

New CI pipeline #1392

New CI pipeline #1392

Conversation

tyvision commented May 29, 2018 • edited Loading

Description of the Change

1 Major changes

1.1 Pipeline stages

1.2 On-demand Jenkins agents

1.3 Merge availability

1.4 CI notifications

1.5 Tests launch separation

1.6 Default order of execution

2 Minor changes

Benefits

Usage Examples or Tests [optional]

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyvision May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muratovv Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muratovv Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muratovv Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyvision Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebdron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyvision commented May 29, 2018 •

edited

Loading

tyvision May 30, 2018 •

edited

Loading

muratovv Jun 4, 2018 •

edited

Loading

muratovv Jun 4, 2018 •

edited

Loading

muratovv Jun 4, 2018 •

edited

Loading

tyvision Jun 6, 2018 •

edited

Loading