Add artifactory support #2272

jdekonin · 2018-06-26T19:12:02Z

adds the capability of reducing footprint on jenkins jobs
concerning the storage of artifacts

Signed-off-by: Joe deKoning joe_dekoning@ca.ibm.com

jdekonin · 2018-06-26T19:12:42Z

AdamBrousseau · 2018-06-27T02:15:43Z

buildenv/jenkins/common/build

+
+              sh "tar -C build/$RELEASE/images/ -zcvf $SDK_FILENAME ${JDK_FOLDER}"
+              if (ARTIFACTORY_SERVER) {
+                echo "ARTIFACTORY server to publish artifacts is: $ARTIFACTORY_SERVER"


Since most of your variables are wrapped with braces, can you wrap the rest of them to be consistent.

AdamBrousseau · 2018-06-27T02:16:17Z

buildenv/jenkins/common/build

+              sh "tar -C build/$RELEASE/images/ -zcvf $SDK_FILENAME ${JDK_FOLDER}"
+              if (ARTIFACTORY_SERVER) {
+                echo "ARTIFACTORY server to publish artifacts is: $ARTIFACTORY_SERVER"
+                def server = Artifactory.server ARTIFACTORY_SERVER


I think this variable usage is fine though

AdamBrousseau · 2018-06-27T02:17:09Z

buildenv/jenkins/common/build

+                echo "Artifactory Upload: $ARTIFACTORY_REPO/${env.JOB_NAME}/${env.BUILD_ID}/"
+
+                def buildInfo = Artifactory.newBuildInfo()
+                buildInfo.retention maxBuilds: 3, deleteBuildArtifacts: true


3 seems small. I assume this was your WIP number.

AdamBrousseau · 2018-06-27T02:19:36Z

buildenv/jenkins/common/build

+		// temporarily copy to jenkins until consuming jobs have been updated to pull from artifactory
+                archiveArtifacts artifacts: "**/${TEST_PREFIX}*${TEST_SUFFIX}", fingerprint: true, onlyIfSuccessful: true
+                archiveArtifacts artifacts: "**/${SDK_PREFIX}*${SDK_SUFFIX}", fingerprint: true, onlyIfSuccessful: true
+              } else {


Since we are archiving to Jenkins in both cases, we technically don't need an else yet. Removing it would save defining the archive twice. Once we stop doing both, then we would need the else. Tom-eh-toh/Tom-a-toh I guess.

AdamBrousseau · 2018-06-27T02:22:09Z

buildenv/jenkins/common/test

+                def downloadSpec = """{
+                    "files": [
+                        {
+                            "pattern": "$ARTIFACTORY_REPO/${params.UPSTREAM_JOB_NAME}/${params.UPSTREAM_JOB_NUMBER}/*.tar.gz",


I was wondering if we should move the Dir tree to the Variable file since we use it twice. But since we are switching test files to Adopt-test, I assume we would pass them the pattern as a job param. Maybe we should pass it now, rather than hardcode the path twice?

AdamBrousseau · 2018-06-27T02:23:37Z

buildenv/jenkins/common/variables-functions

@@ -303,6 +313,9 @@ def set_job_variables(job_type) {
    // fetch credentials required to connect to GitHub using ssh protocol
    USER_CREDENTIALS_ID = get_git_user_credentials_id()

+    set_artifactory_server()
+    set_artifactory_repo()
+


Can these be combined into 1 function? Is there a use case where we would only call one or the other?

jdekonin · 2018-08-14T22:23:55Z

This is ready for review. @AdamBrousseau @llxia

AdamBrousseau

Ref co-change adoptium/aqa-tests#512

AdamBrousseau · 2018-08-15T15:00:00Z

buildenv/jenkins/common/build

+                  returnStdout: true
+              ).trim()
+              TEST_FILENAME = "${TEST_PREFIX}${TESTSHA}${TEST_SUFFIX}"
+              sh "tar -zcvf ${TEST_FILENAME} openj9/test/"


I was wondering if this could be removed but we still have a few jobs not using the Adopt test scripts

I was going to clarify that with @llxia that the test.jar isn't required anymore. I believe the only jobs that depend on that any longer are JDK9 which @pshipton has asked to be turned off from nightly builds.

Update: There are also a few windows jobs, but those need to be updated to work in the openjdk-test manner.

AdamBrousseau · 2018-08-15T15:01:24Z

buildenv/jenkins/common/build

-            archiveArtifacts artifacts: "**/${TEST_PREFIX}*${TEST_SUFFIX}", fingerprint: true, onlyIfSuccessful: true
-            sh "tar -C build/$RELEASE/images/ -zcvf ${SDK_PREFIX}`date +%Y%d%m%H%M`${SDK_SUFFIX} ${JDK_FOLDER}"
-            archiveArtifacts artifacts: "**/${SDK_PREFIX}*${SDK_SUFFIX}", fingerprint: true, onlyIfSuccessful: true
+              SDK_FILENAME = "${SDK_PREFIX}${env.BUILD_ID}${SDK_SUFFIX}"


Is there a reason why we are switching from date +%Y%d%m%H%M to BUILD_ID?

I can revert, I did that to keep the filename simple. The upload and directory naming need a conversation at a broader level once put into use. Having a datestamp in the filename attached as a jenkins artifact is irrelevant as all that information is attached to the job.

AdamBrousseau · 2018-08-15T15:04:03Z

buildenv/jenkins/common/build

+                def uploadSpec = """{
+                  "files":[
+                    {
+                      "pattern": "${env.WORKSPACE}/*gz",


Should we use SDK_SUFFIX instead of hardcoding *gz? Since TEST_SUFFIX is the same and will be disappearing soon. When we remove the test stuff this could become SDK_FILENAME too

AdamBrousseau · 2018-08-15T15:05:12Z

buildenv/jenkins/common/build

+                }"""
+
+                def buildInfo = Artifactory.newBuildInfo()
+                // hardcoded to 20 for now, this need fup for usage on per platform level or across all jobs


typo? fup

AdamBrousseau · 2018-08-15T15:06:01Z

buildenv/jenkins/common/build

+
+                def buildInfo = Artifactory.newBuildInfo()
+                // hardcoded to 20 for now, this need fup for usage on per platform level or across all jobs
+                buildInfo.retention maxBuilds: 30, deleteBuildArtifacts: true


Just to confirm, this retention policy is on a per job name basis correct?

AdamBrousseau · 2018-08-15T15:08:27Z

buildenv/jenkins/common/pipeline-functions

-                jobs["${TEST_JOB_NAME}"] = build_with_one_upstream(TEST_JOB_NAME, BUILD_JOB_NAME, jobs["build"].getNumber(), params.VARIABLE_FILE, params.VENDOR_REPO, params.VENDOR_BRANCH, params.VENDOR_CREDENTIALS_ID, params.TEST_NODE, OPENJ9_REPO, OPENJ9_BRANCH, SHAS['OPENJ9'], VENDOR_TEST_REPOS, VENDOR_TEST_BRANCHES, VENDOR_TEST_SHAS, VENDOR_TEST_DIRS, USER_CREDENTIALS_ID, BUILD_LIST)
+                     if (ARTIFACTORY_SERVER) {
+                          jobs["${TEST_JOB_NAME}"] = build_with_one_upstream(TEST_JOB_NAME, "", "", params.VARIABLE_FILE, params.VENDOR_REPO, params.VENDOR_BRANCH, params.VENDOR_CREDENTIALS_ID, params.TEST_NODE, OPENJ9_REPO, OPENJ9_BRANCH, SHAS['OPENJ9'], VENDOR_TEST_REPOS, VENDOR_TEST_BRANCHES, VENDOR_TEST_SHAS, VENDOR_TEST_DIRS, USER_CREDENTIALS_ID, BUILD_LIST, CUSTOMIZED_SDK_URL, ARTIFACTORY_CREDS)
+                     } else { 


Is this if else necessary? Do the Artifactory variables get set to null and can we pass them no matter if they have a real value or not?

Edit: Now I see you also pass "" for the UPSTREAM_JOB* variables. I suppose this works but we can simplify it once we fully switch to Adopt test scripts.

Edit: Its not about Adopt test scripts, its about supporting both Artifactory and Master archiving. I wonder if it should be calling a different function that has slightly different params?

AdamBrousseau · 2018-08-15T15:09:17Z

buildenv/jenkins/common/pipeline-functions

@@ -217,6 +219,7 @@ def workflow(SDK_VERSION, SPEC, SHAS, OPENJDK_REPO, OPENJDK_BRANCH, OPENJ9_REPO,
    def BUILD_JOB_NAME = "Build-JDK${SDK_VERSION}-${SPEC}"
    jobs["build"] = build(BUILD_JOB_NAME, OPENJDK_REPO, OPENJDK_BRANCH, SHAS['OPENJDK'], OPENJ9_REPO, OPENJ9_BRANCH, SHAS['OPENJ9'], OMR_REPO, OMR_BRANCH, SHAS['OMR'], params.VARIABLE_FILE, params.VENDOR_REPO, params.VENDOR_BRANCH, params.VENDOR_CREDENTIALS_ID, params.BUILD_NODE)
    echo "JOB: ${BUILD_JOB_NAME} PASSED in: ${jobs['build'].getDurationString()}"
+    def CUSTOMIZED_SDK_URL = "${ARTIFACTORY_BASE_URL}/${BUILD_JOB_NAME}/${jobs["build"].getNumber()}/OpenJ9-JDK${SDK_VERSION}-${SPEC}-${jobs["build"].getNumber()}.tar.gz"


Is SDK_NAME available at this point in time? ref back to my comment about changing from date to BUILD_ID

AdamBrousseau · 2018-08-15T15:15:24Z

buildenv/jenkins/common/variables-functions

@@ -394,6 +404,7 @@ def set_job_variables(job_type) {

    // fetch credentials required to connect to GitHub using ssh protocol
    set_user_credentials()
+    set_artifactory_config()


I think this technically only needs to be done for case "build" or "test"? Is it worth moving it? Can likely remove it from test case too once we only use Adopt test scripts.

AdamBrousseau · 2018-08-15T15:35:16Z

May need to figure out tagging levels shortly after we merge this. Anyone wanting to pull "nightlies" will need to pull the right level and not a developer build.

AdamBrousseau · 2018-08-16T04:11:06Z

this is in regards to the build system pulling specific builds

No, the build system is fine and will know what to pull. My comment is regarding anyone, manually or automatically, wanting to pull binaries outside of our build system for other testing purposes. They currently won't be able to determine what "build" they're getting.

jdekonin · 2018-08-29T17:25:58Z

@AdamBrousseau I believe this is now ready for review. I can walk you through the testing that I performed in sandbox jobs if you want to see the changes in action.

AdamBrousseau

Looks good on initial pass. Will looks at the test cases next

AdamBrousseau · 2018-08-29T19:18:19Z

buildenv/jenkins/common/variables-functions

+
+        SDK_SUFFIX = ".tar.gz"
+        TEST_PREFIX = "test-"
+        TEST_SUFFIX = ".tar.gz"


These are no longer needed

AdamBrousseau · 2018-08-29T19:29:07Z

buildenv/jenkins/common/variables-functions

+    SDK_FILENAME = params.SDK_FILENAME
+    if (!SDK_FILENAME) {
+        try{
+            SDK_PREFIX = "OpenJ9-JDK${SDK_VERSION}-PR${ghprbPullId}-${SPEC}-"


We don't archive PRs anymore so we might as well remove this while we're here. Can likely merge Prefix and Suffix into the SDK_FILENAME declaration

Changes made in latest push

AdamBrousseau

Some minor nits, other than that LGTM.

AdamBrousseau · 2018-08-30T03:09:18Z

buildenv/jenkins/common/build

+
+                def buildInfo = Artifactory.newBuildInfo()
+                // hardcoded to 30 for now, this need follow up for usage on per platform level or across all jobs
+                buildInfo.retention maxBuilds: 30, deleteBuildArtifacts: true


Can probably use this once its ready (just fyi, not blocking this on that)
https://github.com/eclipse/openj9/pull/2138/files#diff-86d1cc0920536c6d4819089e4772aa71R472
https://github.com/eclipse/openj9/pull/2138/files#diff-a3fca431c8c372cebd1caff040ca6eabR59

AdamBrousseau · 2018-08-30T03:14:06Z

buildenv/jenkins/common/variables-functions

-    TEST_PREFIX = "test-"
-    TEST_SUFFIX = ".tar.gz"
-
+


some whitespaces here

AdamBrousseau · 2018-08-30T03:14:35Z

buildenv/jenkins/common/pipeline-functions

-                jobs["${TEST_JOB_NAME}"] = build_with_one_upstream(TEST_JOB_NAME, BUILD_JOB_NAME, jobs["build"].getNumber(), params.VARIABLE_FILE, params.VENDOR_REPO, params.VENDOR_BRANCH, params.VENDOR_CREDENTIALS_ID, params.TEST_NODE, OPENJ9_REPO, OPENJ9_BRANCH, SHAS['OPENJ9'], VENDOR_TEST_REPOS, VENDOR_TEST_BRANCHES, VENDOR_TEST_SHAS, VENDOR_TEST_DIRS, USER_CREDENTIALS_ID, BUILD_LIST)
+                     if (ARTIFACTORY_SERVER) {
+                          def CUSTOMIZED_SDK_URL = "${ARTIFACTORY_BASE_URL}/${BUILD_JOB_NAME}/${jobs["build"].getNumber()}/${SDK_FILENAME}"
+                          echo "Using CUSTOMIZED_SDK_URL = ${CUSTOMIZED_SDK_URL}, ARTIFACTORY_CREDS = ${ARTIFACTORY_CREDS}" 


extra whitespace at the end of the line

AdamBrousseau · 2018-08-30T03:14:42Z

buildenv/jenkins/common/pipeline-functions

+                          def CUSTOMIZED_SDK_URL = "${ARTIFACTORY_BASE_URL}/${BUILD_JOB_NAME}/${jobs["build"].getNumber()}/${SDK_FILENAME}"
+                          echo "Using CUSTOMIZED_SDK_URL = ${CUSTOMIZED_SDK_URL}, ARTIFACTORY_CREDS = ${ARTIFACTORY_CREDS}" 
+                          jobs["${TEST_JOB_NAME}"] = build_with_artifactory(TEST_JOB_NAME, params.VARIABLE_FILE, params.VENDOR_REPO, params.VENDOR_BRANCH, params.VENDOR_CREDENTIALS_ID, params.TEST_NODE, OPENJ9_REPO, OPENJ9_BRANCH, SHAS['OPENJ9'], VENDOR_TEST_REPOS, VENDOR_TEST_BRANCHES, VENDOR_TEST_SHAS, VENDOR_TEST_DIRS, USER_CREDENTIALS_ID, BUILD_LIST, CUSTOMIZED_SDK_URL, ARTIFACTORY_CREDS)
+                     } else { 


extra whitespace at the end of the line

AdamBrousseau · 2018-08-30T03:14:58Z

buildenv/jenkins/common/build

+                // hardcoded to 30 for now, this need follow up for usage on per platform level or across all jobs
+                buildInfo.retention maxBuilds: 30, deleteBuildArtifacts: true
+                server.upload spec: uploadSpec, buildInfo: buildInfo
+                server.publishBuildInfo buildInfo 


extra whitespace at the end of the line

AdamBrousseau · 2018-08-30T03:18:03Z

buildenv/jenkins/common/variables-functions

+            returnStdout: true
+            ).trim()
+        SDK_FILENAME = "OpenJ9-JDK${SDK_VERSION}-${SPEC}-${DATESTAMP}.tar.gz"
+    }


Wouldn't hurt to add an echo of the filename here outside the IF

llxia · 2018-08-30T13:25:24Z

buildenv/jenkins/common/build

+                  "files":[
+                    {
+                      "pattern": "${env.WORKSPACE}/${SDK_FILENAME}",
+                      "target": "${ARTIFACTORY_REPO}/${env.JOB_NAME}/${env.BUILD_ID}/"


I would suggest to add Jenkins server name. For example, ${ARTIFACTORY_REPO}/${Jenkins_Server}/${env.JOB_NAME}/${env.BUILD_ID}/

With different Jenkins servers uploading to the same Artifactory, if we only identify them by job name and build id, we will have override issue.

I think Joe decided to hardcode it in the Variable file. By default, JENKINS_URL is a full https url so you need to parse it in order get "JENKINS_SERVER". The latter would be more future proof. I believe I've seen your code for this somewhere.

yes, JENKINS_URL is a full https url which is not ideal to use as folder name. Below is the code that I used before.

def strs = env.HUDSON_URL.split( ':' ) def url = strs[1].split( '//' ) def serverName = "undefined" if (url && url.length>1) { serverName = url[1] }

I think that is enforcing a directory structure through code that might not be desired. Setting that via the variable file promotes flexibility of use. Example...

#========================================# # Artifactory settings #========================================# artifactory_server: 'artifactoryserver.companyname.com artifactory_repo: '<repo name>[/<jenkins server or other directory naming if desired>]' artifactory_creds: 'acbd1234-a1b2-1234-5678-123abc456def'

What if we had a combination of the 2?
i.e. Write a function to populate JENKINS_SERVER as a global variable. Then use the variable name in the yml file.

Yes, I am ok with it. All I am saying is that by including Jenkins server name, we will be more future proof. I have already encountered similar issue in Test Result Summary Service, where we need to monitor two Jenkins servers and they have same build names. :)

Talked to Joe. Came to the understanding that the current directory structure is temporary and we are not sure how it will change. I think its fine to leave the change as is for now and rework it as needed when we decide how we're going to change it.

* adds the capability of reducing footprint on jenkins jobs concerning the storage of artifacts * [skip ci] Signed-off-by: Joe deKoning <joe_dekoning@ca.ibm.com>

jdekonin · 2018-08-30T16:06:04Z

Last push 29f1050 is a rebase of master plus the minor whitespace and echo output @AdamBrousseau pointed out.

Related eclipse-openj9#2272 [skip ci] Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>

- We no longer run our own testing so this code is no longer needed. - Remove PullRequest pipelines as they are no longer in use. - Combine 'build' and 'pullRequest' variable setup cases as they are identical. - Also remove temp reverse spec map used prior to eclipse-openj9#2836. Related eclipse-openj9#1450 Depends eclipse-openj9#2728 eclipse-openj9#2638 eclipse-openj9#2467 eclipse-openj9#2272 eclipse-openj9#2836 eclipse-openj9#4443 [skip ci] Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>

AdamBrousseau requested changes Jun 27, 2018

View reviewed changes

pshipton added the comp:build label Jun 27, 2018

jdekonin force-pushed the artifactory branch from cd64238 to ae1fea6 Compare June 27, 2018 16:39

jdekonin force-pushed the artifactory branch from ae1fea6 to 8de0632 Compare August 8, 2018 20:57

jdekonin changed the title ~~Add artifactory support~~ WIP: Add artifactory support Aug 8, 2018

jdekonin force-pushed the artifactory branch 20 times, most recently from 45eabfb to 2a1ed9a Compare August 14, 2018 22:22

jdekonin changed the title ~~WIP: Add artifactory support~~ Add artifactory support Aug 14, 2018

AdamBrousseau reviewed Aug 15, 2018

View reviewed changes

AdamBrousseau mentioned this pull request Aug 16, 2018

Investigate Options for Jenkins artifact Storage #2637

Open

1 task

AdamBrousseau mentioned this pull request Aug 29, 2018

Find and archive any core / dmp files in post test step #969

Closed

AdamBrousseau mentioned this pull request Aug 29, 2018

Remove obsolete pipeline code #2743

Merged

jdekonin force-pushed the artifactory branch from 2a1ed9a to 93db3d6 Compare August 29, 2018 17:23

AdamBrousseau reviewed Aug 29, 2018

View reviewed changes

jdekonin force-pushed the artifactory branch from 93db3d6 to 6ad9eef Compare August 29, 2018 20:28

AdamBrousseau approved these changes Aug 30, 2018

View reviewed changes

llxia reviewed Aug 30, 2018

View reviewed changes

Add artifactory support

29f1050

* adds the capability of reducing footprint on jenkins jobs concerning the storage of artifacts * [skip ci] Signed-off-by: Joe deKoning <joe_dekoning@ca.ibm.com>

jdekonin force-pushed the artifactory branch from 6ad9eef to 29f1050 Compare August 30, 2018 16:04

llxia approved these changes Sep 5, 2018

View reviewed changes

llxia merged commit b36fb07 into eclipse-openj9:master Sep 5, 2018

AdamBrousseau added a commit to AdamBrousseau/openj9 that referenced this pull request Sep 5, 2018

Set SDK variables in the OMR build

6ac3dee

Related eclipse-openj9#2272 [skip ci] Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>

This was referenced Sep 5, 2018

WIP: Set SDK variables in the OMR build #2777

Closed

Convert OMR Acceptance build to use Pipeline-Build-Test-All #2778

Closed

AdamBrousseau mentioned this pull request Sep 27, 2018

Convert PullRequests to pipeline of jobs #2836

Merged

jdekonin deleted the artifactory branch December 11, 2019 17:30

Add artifactory support #2272

Add artifactory support #2272

Conversation

jdekonin commented Jun 26, 2018

jdekonin commented Jun 26, 2018

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

jdekonin commented Aug 14, 2018

AdamBrousseau left a comment

Choose a reason for hiding this comment

AdamBrousseau Aug 15, 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

AdamBrousseau Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamBrousseau commented Aug 15, 2018

AdamBrousseau commented Aug 16, 2018

jdekonin commented Aug 29, 2018

AdamBrousseau left a 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

AdamBrousseau left a 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

jdekonin commented Aug 30, 2018

AdamBrousseau Aug 15, 2018 •

edited

Loading

AdamBrousseau Aug 15, 2018 •

edited

Loading