-
Notifications
You must be signed in to change notification settings - Fork 60
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.
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
Updating platform jobs for pluggable scm #30
Conversation
// Setup setup_cartridge | ||
setupPluggable.with{ | ||
environmentVariables { | ||
keepBuildVariables() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a "true" in the brackets: https://jenkinsci.github.io/job-dsl-plugin/#path/freeStyleJob-environmentVariables-keepBuildVariables
setupPluggable.with{ | ||
environmentVariables { | ||
keepBuildVariables() | ||
keepSystemVariables() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a "true" in the brackets: https://jenkinsci.github.io/job-dsl-plugin/#path/freeStyleJob-environmentVariables-keepSystemVariables
4e00f6f
to
baeb703
Compare
Thanks for the review @nickdgriffin I have fixed that bug now. Waiting for this PR to get merged in: dsingh07#1 to my branch, and this PR should be in a reviewable state again. |
There have been changes introduced by @BuleGeorge and @RobertNorthard which will be tested by me, after rebasing with all the other changes that have been introduced. I shall update this PR with exciting developments :) |
4f48dbd
to
f3ebf7d
Compare
projects/jobs/jobs.groovy
Outdated
systemGroovyCommand(''' | ||
import jenkins.model.* | ||
import groovy.io.FileType | ||
>>>>>>> Updating platform jobs for pluggable scm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be trimmed out please?
00741ee
to
333de5f
Compare
projects/jobs/jobs.groovy
Outdated
# Check if SCM namespace is specified | ||
if [ -z ${SCM_NAMESPACE} ] ; then | ||
echo "SCM_NAMESPACE not specified, setting to PROJECT_NAME..." | ||
SCM_NAMESPACE="${PROJECT_NAME}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SCM_NAMESPACE="${PROJECT_NAME}/${CARTRIDGE_FOLDER}" if cartridge folder has been specified. If not I suspect it will just be empty and default to using the project name.
projects/jobs/jobs.groovy
Outdated
|
||
# Find the cartridge | ||
export CART_HOME=$(dirname $(find -name metadata.cartridge | head -1)) | ||
|
||
echo "CART_HOME=${CART_HOME}" > ${WORKSPACE}/carthome.properties | ||
|
||
# Check if the user has enabled Gerrit Code reviewing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can we remove all Gerrit references.
projects/jobs/jobs.groovy
Outdated
|
||
def workspace = envVarProperty.getProperty('WORKSPACE') | ||
def projectFolderName = envVarProperty.getProperty('PROJECT_NAME') | ||
def cartridgeFolder = envVarProperty.getProperty('CARTRIDGE_FOLDER') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If CARTRIDGE_FOLDER is not provided we will get a missing property exception. I propose we do something similar to the SCM_NAMESPACE below and default it to '' if it's not provided.
Thanks for the quick review @RobertNorthard Have made those changes that you've requested. |
885e3a0
to
86f3e22
Compare
Thanks @dsingh07. @nickdgriffin @anton-kasperovich this requires one final look over (fresh pair of eyes) before we merge. |
86f3e22
to
975bdb6
Compare
Signed-off-by: Robert A. Nothard <[email protected]>
Signed-off-by: Northard, Robert A <[email protected]>
975bdb6
to
b56ab4a
Compare
Commits have been squashed and is ready for a final round of testing from @nickdgriffin and @anton-kasperovich :) |
projects/jobs/jobs.groovy
Outdated
@@ -2,10 +2,20 @@ | |||
def gerritBaseUrl = "ssh://jenkins@gerrit:29418" | |||
def cartridgeBaseUrl = gerritBaseUrl + "/cartridges" | |||
def platformToolsGitUrl = gerritBaseUrl + "/platform-management" | |||
def scmPropertiesPath = "${PLUGGABLE_SCM_PROVIDER_PROPERTIES_PATH}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable looks like it's not used. Please can we remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Thanks @RobertNorthard
Thanks @dsingh07 added a few comments in relation to error handling. After this PR is merged, we should also merge Accenture/adop-cartridge-skeleton#3 to demonstrate how to use the pluggable library. |
projects/jobs/jobs.groovy
Outdated
|
||
credentialInfo = [CredentialsMatchers.firstOrNull(available_credentials, username_matcher).username, | ||
CredentialsMatchers.firstOrNull(available_credentials, username_matcher).password]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will throw an unchecked exception (null pointer) if the credentials are not found.
java.lang.NullPointerException: Cannot get property 'username' on null object".
Please can we wrap the username/password extraction so if it throws a NPE a helpful warning message is displayed to the user and default the username/password to an empty string.
The job should not fail if credentials are not found beacuse:
- the credentials are only required by the SCM create repo section of the cartridge loader
- not all cartridges will utilise urls.txt as they are just using the SCM provider to generate the appropriate SCM get/triggers.
…arning message to indicate credentials have not been provided. Signed-off-by: Northard, Robert A <[email protected]>
Changes made by @RobertNorthard
d8af25d
to
b5ccea7
Compare
LGTM. Thanks everyone for the hard work. @dsingh07 @nickdgriffin @anton-kasperovich @BuleGeorge @SachinKSingh28 @marisbahtins @IrmantasM |
This pull request will update miscellaneous platform jobs to support Pluggable SCM:
Test Scenarios
** Expect spring petclinic repos to be created in Gerrit
As the build label is also set to !master the above tests should also confirm we can run the cartridge loader from unix/linux based slaves.
For all tests it is expected the respective repositories captured in urls.txt will be created with the respective SCM provider (Gerrit, BitBucket)
This PR is dependent on:
Accenture/adop-jenkins#29Accenture/adop-docker-compose#187Accenture/adop-pluggable-scm#4