-
Notifications
You must be signed in to change notification settings - Fork 23
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
Create a PR main.tf template and use tfvars to specify test envs #898
Conversation
Create template main.tf for all PR environments and manage specific env variables.Why ?I revamp our main.tf configuration for the PR deployment. It's something I wanted to address for a while. This new pipeline is probably the right opportunity. Normally in terraform project, it's better to separate The first step is to remove all variable declarations from main.tf and move it to a variable.tf file ( this file has to be in the same main.tf directory).
I isolated 4 specific environment variables :
To manage this variables, I extracted the hard written variables from the main.tf and added it to the variable.tf. First solution ( folder
|
Create tfvars to manage uyuni and spacewalk configurationFor this variable we will still use the template main tf configuration. If more variables where to be identified, we can easily add its to this file FYI, overwrite order in terraform : |
Manage variables directly parse during terraform call :
${fqdn_jenkins_node} => hostname -f done during pipeline =>
|
@@ -0,0 +1,14 @@ | |||
############ Variable unique to uyuni ######################## | |||
|
|||
IMAGE = "opensuse154-ci-pro" |
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 wasn't aware of this image, what makes it different?
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.
@jordimassaguerpla can give an explanation on this. FMPOV it simply is an adjusted image with already preinstalled packages and adjusted configuration. See https://build.opensuse.org/package/show/systemsmanagement:sumaform:images:libvirt/opensuse154-ci-pr
############ Varaibles unique to spacewalk ########### | ||
|
||
IMAGE = "sles15sp4o" | ||
GIT_PROFILES_REPO = "https://github.com/SUSE/spacewalk.git#:testsuite/features/profiles/internal_prv" |
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.
For that variable, we will need to diverge it depending of the environment is placed in PRV or NUE. Otherwise the retail tests might fail. But we can let this for a next iteration.
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.
Very nice refactor by using this PR-TEST-main.tf 💯
My only nitpick is that I would not use uppercase in the filename, but this looks great, thank you.
non_os_pool = "http://${var.MIRROR}/distribution/leap/15.4/repo/non-oss/", | ||
os_pool = "http://${var.MIRROR}/distribution/leap/15.4/repo/oss/", |
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 miss something or we are not parametrizing between leap and sles, for Uyuni and SUMA PRs envs?
os_pool = "http://${var.MIRROR}/distribution/leap/15.4/repo/oss/", | ||
os_update = var.UPDATE_REPO, | ||
os_additional_repo = var.ADDITIONAL_REPO_URL, | ||
testing_overlay_devel = "http://${var.MIRROR}/repositories/systemsmanagement:/Uyuni:/Master/images/repo/Testing-Overlay-POOL-x86_64-Media1/", |
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 miss something or we are not parametrizing between Uyuni and SUMA images for the testing overlay devel?
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.
All repositories will need a revamp too. But I will need Jordi for that. It's just a first step, we will probably still move parameters around. For now, my main focus is to keep the deployment working with all the changes.
non_os_pool = "http://${var.MIRROR}/distribution/leap/15.4/repo/non-oss/", | ||
os_pool = "http://${var.MIRROR}/distribution/leap/15.4/repo/oss/", |
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.
Same than on the server module
testing_overlay_devel = "http://${var.MIRROR}/repositories/systemsmanagement:/Uyuni:/Master/images/repo/Testing-Overlay-POOL-x86_64-Media1/", | ||
proxy_pool = "http://${var.MIRROR}/repositories/systemsmanagement:/Uyuni:/Master/images/repo/Uyuni-Proxy-POOL-x86_64-Media1/", |
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.
Same than on the server module
@@ -0,0 +1,208 @@ | |||
def run(params) { |
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 guess this "-light" pipeline is just temporary, and we will replace the content of this one inside "pipeline-pull-request.groovy" once it's stable?
Otherwise, we will have an extra pipeline, and a older one not anymore in use.
jenkins_pipelines/environments/common/pipeline-pull-request-light.groovy
Outdated
Show resolved
Hide resolved
jenkins_pipelines/environments/common/pipeline-pull-request-light.groovy
Outdated
Show resolved
Hide resolved
} | ||
|
||
// Clean up old results | ||
sh "if [ -d ${resultdir} ];then ./clean-old-results -r ${resultdir};fi" |
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.
Being able to have if statements at Groovy level, I don't see a good reason to delegate it to bash.
jenkins_pipelines/environments/common/pipeline-pull-request-light.groovy
Outdated
Show resolved
Hide resolved
jenkins_pipelines/environments/common/pipeline-pull-request-light.groovy
Outdated
Show resolved
Hide resolved
jenkins_pipelines/environments/common/pipeline-pull-request-light.groovy
Outdated
Show resolved
Hide resolved
jenkins_pipelines/environments/common/pipeline-pull-request-light.groovy
Outdated
Show resolved
Hide resolved
jenkins_pipelines/environments/common/pipeline-pull-request-light.groovy
Outdated
Show resolved
Hide resolved
Maybe instead of having the need of provide one by one all the files for each kind of terraform variables that terraform can handle. |
bb568c6
to
738e1b8
Compare
2a5424d
to
f753b51
Compare
Currently running on https://ci.suse.de/view/Manager/view/Uyuni-PRs/job/uyuni-prs-ci-tests-jordi/ |
############ Spacewalk unique variables ############ | ||
|
||
IMAGE = "sles15sp4o" | ||
IMAGES = ["rocky9o", "opensuse154o", "sles15sp4o", "ubuntu2204o"] |
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.
Rocky9 is correct?
I though we were testing Rocky8 in 4.3 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.
In SUSE-Manager-4.3-PR-tests-env5.tf, I can see we are using rocky9
sh "echo \"############ Repositories variables ############\" >> ${env.resultdir}/sumaform/terraform.tfvars" | ||
sh "echo PULL_REQUEST_REPO = \\\"http://${fqdn_jenkins_node}/workspace/${short_product_name}-pr${env_number}/repos/${builder_project}:${pull_request_number}/${build_repo}/${arch}\\\" >> ${env.resultdir}/sumaform/terraform.tfvars" | ||
sh "echo MASTER_REPO = \\\"http://${fqdn_jenkins_node}/workspace/${short_product_name}-pr${env_number}/repos/${source_project}/${build_repo}/${arch}\\\" >> ${env.resultdir}/sumaform/terraform.tfvars" | ||
sh "echo MASTER_OTHER_REPO = \\\"http://${fqdn_jenkins_node}/workspace/${short_product_name}-pr${env_number}/repos/${other_project}/${other_build_repo}/${arch}\\\" >> ${env.resultdir}/sumaform/terraform.tfvars" | ||
sh "echo MASTER_SUMAFORM_TOOLS_REPO = \\\"http://${fqdn_jenkins_node}/workspace/${short_product_name}-pr${env_number}/repos/${sumaform_tools_project}/${build_repo}/${arch}\\\" >> ${env.resultdir}/sumaform/terraform.tfvars" | ||
sh "echo TEST_PACKAGES_REPO = \\\"http://${fqdn_jenkins_node}/workspace/${short_product_name}-pr${env_number}/repos/${test_packages_project}/rpm/${arch}\\\" >> ${env.resultdir}/sumaform/terraform.tfvars" | ||
sh "echo UPDATE_REPO = \\\"${update_repo}\\\" >> ${env.resultdir}/sumaform/terraform.tfvars" |
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.
In my opinion, to avoid that many escaped characters it could be good to keep using env.XXXX
And then, you do something like:
sh "echo MASTER_SUMAFORM_TOOLS_REPO = ${env.MASTER_SUMAFORM_TOOLS_REPO}" >> ${env.resultdir}/sumaform/terraform.tfvars
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 will try but I think we probably still need the extra quote ( terraform format)
env.TEST_PACKAGES_REPO = "http://${fqdn_jenkins_node}/workspace/${short_product_name}-pr${env_number}/repos/${test_packages_project}/rpm/${arch}" | ||
env.UPDATE_REPO = update_repo | ||
sh "echo \"############ Repositories variables ############\" >> ${env.resultdir}/sumaform/terraform.tfvars" | ||
sh "echo PULL_REQUEST_REPO = \\\"http://${fqdn_jenkins_node}/workspace/${short_product_name}-pr${env_number}/repos/${builder_project}:${pull_request_number}/${build_repo}/${arch}\\\" >> ${env.resultdir}/sumaform/terraform.tfvars" |
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 did not check, but I would say we need to escape other characters inside that URL 🤔
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.
Not it works correctly :
PULL_REQUEST_REPO = "http://jenkins-worker-prs.mgr.prv.suse.net/workspace/suma-pr10/repos/systemsmanagement:Uyuni:Master:PR:8067/openSUSE_Leap_15.5/x86_64"
MASTER_REPO = "http://jenkins-worker-prs.mgr.prv.suse.net/workspace/suma-pr10/repos/systemsmanagement:Uyuni:Master/openSUSE_Leap_15.5/x86_64"
MASTER_OTHER_REPO = "http://jenkins-worker-prs.mgr.prv.suse.net/workspace/suma-pr10/repos/systemsmanagement:Uyuni:Master:Other/openSUSE_Leap_15.5/x86_64"
MASTER_SUMAFORM_TOOLS_REPO = "http://jenkins-worker-prs.mgr.prv.suse.net/workspace/suma-pr10/repos/systemsmanagement:sumaform:tools/openSUSE_Leap_15.5/x86_64"
TEST_PACKAGES_REPO = "http://jenkins-worker-prs.mgr.prv.suse.net/workspace/suma-pr10/repos/systemsmanagement:Uyuni:Test-Packages:Pool/rpm/x86_64"
UPDATE_REPO = "http://minima-mirror-ci-bv.mgr.prv.suse.net/jordi/some-updates/"
ADDITIONAL_REPO_URL = "http://minima-mirror-ci-bv.mgr.prv.suse.net/jordi/dummy/"
SLE_CLIENT_REPO = "http://jenkins-worker-prs.mgr.prv.suse.net/workspace/suma-pr10/repos/systemsmanagement:Uyuni:Master:SLE15-Uyuni-Client-Tools/SLE_15/x86_64"
RHLIKE_CLIENT_REPO = "http://jenkins-worker-prs.mgr.prv.suse.net/workspace/suma-pr10/repos/systemsmanagement:Uyuni:Master:EL9-Uyuni-Client-Tools/EL_9/x86_64"
DEBLIKE_CLIENT_REPO = "http://jenkins-worker-prs.mgr.prv.suse.net/workspace/suma-pr10/repos/systemsmanagement:Uyuni:Master:Ubuntu2204-Uyuni-Client-Tools/xUbuntu_22.04/x86_64"
OPENSUSE_CLIENT_REPO = "http://jenkins-worker-prs.mgr.prv.suse.net/workspace/suma-pr10/repos/systemsmanagement:Uyuni:Master:openSUSE_Leap_15-Uyuni-Client-Tools/openSUSE_Leap_15.0/x86_64"
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.
That's a hard one to review. I added few comments.
Once you test and it works well... fine for me to merge. (Be careful with the commits coming from a pull, better to fix it before merging too)
TESTING-DONT-REVIEW-pipeline-pull-request-light.groovy is for testing purpose. DON'T REVIEW
DON"T REVIEW TESTING-DONT-REVIEW-pipeline-pull-request-light.groovy
This PR is a revamp of all main.tf for the PR testing pipelines to manage the differences in tfvars files and have one unique main.tf template common for all the PR testing environments.
Related to https://github.com/SUSE/spacewalk/issues/21471