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
Added job config for unit tests and functional tests with service job #624
Conversation
|
#dotests |
|
#dotests-unittests |
ci/job.yml
Outdated
| sh ./cccp_ci_container_index.sh | ||
|
|
||
| - job: | ||
| name: 'centos-container-pipeline-service-ci-pr-openshift' |
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.
We don't need quotes here. We didn't use quotes in other job names.
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.
Adding quotes is always better in jjb template name to avoid surprises. Added to all the jobs now.
ci/job.yml
Outdated
| - navidshaikh | ||
| cron: '* * * * *' | ||
| github-hooks: false | ||
| permit-all: true |
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.
add
trigger-phrase: '#dotests'
also add other properties here well like in ci-functional ? Like messages when its started, succeed, failed, error etc?
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.
@bamachrn : We need either #dotests or ok to test string right for triggering the CI via comment?
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.
by default it is ok to test. and in this pr we are not changing anything from index CI
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 pointed out to streamline the trigger CI string in comment in both repositories.
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.
Added #dotests for index ci
ci/job.yml
Outdated
| cd ci | ||
| sh trigger_ci.sh ${GIT_URL} ${GIT_BRANCH} ${sha1} | ||
| else | ||
| echo "It is not on openshit branch" |
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.
*The PR's target branch is not openshift.
| - navidshaikh | ||
| - mohammedzee1000 | ||
| - cdrage | ||
| trigger-phrase: '#dotests-unittests' |
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.
👍
| started-status: "centos-ci functional tests started" | ||
| success-status: "centos-ci functional tests succeeded" | ||
| failure-status: "centos-ci functional tests failed" | ||
| error-status: "centos-ci functional tests errored" |
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.
👍
| started-status: "centos-ci unit tests started" | ||
| success-status: "centos-ci unit tests succeeded" | ||
| failure-status: "centos-ci unit tests failed" | ||
| error-status: "centos-ci unit tests errored" |
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.
👍
ci/job.yml
Outdated
| git rebase origin/${ghprbTargetBranch} | ||
| bash ci/ccp_ci_unittests.sh | ||
| else | ||
| echo "It is not on openshit branch" |
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.
*The PR's target branch is not openshift.
|
@bamachrn: can you take a look at review comments and update the PR? |
|
#dotests-unittests |
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.
- Looks good overall. Requested a few changes in terms of doc.
- Can we use jjb templates for repeating code? Not right away but maybe after the prod deployment.
| - openshift | ||
| builders: | ||
| - shell: | | ||
| jenkins-jobs --ignore-cache --conf ~/jenkins_jobs.ini update ci/job.yml |
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.
There's no need to use absolute path here, right?
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 would say we are safer using absolute paths than relative paths, unless they are going to change constantly
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.
here for the jenkins_jobs.ini we are note sure about the absolute path all the time, but we want to use jenkins_job.ini form users home directory. This runs from jenkins workspace, and key is from home directory. So better to not introduce complexity, when jenkins can handle this itself.
ci/job.yml
Outdated
| @@ -0,0 +1,168 @@ | |||
| - job: | |||
| name: 'centos-container-pipeline-service-container-index' | |||
| description: | | |||
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.
Similar request for description as for the service-job.
ci/job.yml
Outdated
| started-status: "centos-ci container-index ci tests started" | ||
| success-status: "centos-ci container-index ci tests succeeded" | ||
| failure-status: "centos-ci container-index ci tests failed" | ||
| error-status: "centos-ci container-index ci tests errored" |
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.
Is it required to have multiple spaces for above four statuses?
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 is kind of typo while copy paste, fixing this.
| builders: | ||
| - shell: | | ||
| git rebase origin/${ghprbTargetBranch} | ||
| curl https://raw.githubusercontent.com/CentOS/container-pipeline-service/master/ci/cccp_ci_container_index.sh > cccp_ci_container_index.sh |
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.
Out of curiosity, how do we handle a situation where the script being downloaded here itself needs to be changed?
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.
Yes, in this situation it will cause an issue for testing before merge. Although this script should not need to change very often, it is a possibility.
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 is container-index CI, and the script itself downloads/clones all the required code base. So, whenever there is a change in the script, it will be tracked with the container-pipeline ci, not this one. @mohammedzee1000 I believe you can give us better explanation, if required on this.
ci/job.yml
Outdated
| @@ -0,0 +1,168 @@ | |||
| - job: | |||
| name: 'centos-container-pipeline-service-container-index' | |||
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.
Is it just me or it's actually confusing that the name has both "Container Pipeline Service" and "Container Index"? Which one exactly does it do CI for?
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 we could prepend our CI jobs with ccp if that's the reason why we have mentioned "centos-container-pipeline-service" in the 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.
updated to centos-container-index-ci.
ci/job.yml
Outdated
| cd ci | ||
| sh trigger_ci.sh ${GIT_URL} ${GIT_BRANCH} ${sha1} | ||
| else | ||
| echo "PRs target branch is not openshift" |
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.
s/PRs/PR's/
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.
LGTM
This PR contains job configs for
All the test jobs updates their status with a different status-context which shows up in github as multiple checks