Skip to content

Conversation

@floatingman
Copy link
Contributor

Introduce shared library functions for Tofu and Ansible operations, enabling execution within Docker containers. Enhance AWS credentials handling and SSH key management for improved security and functionality. Implement infrastructure helpers for configuration management and workspace handling.

- Add tofu.groovy: Functions for backend init, workspace management, apply/destroy
- Add ansible.groovy: Functions for running playbooks and managing inventory
- Add infrastructure.groovy: Helper functions for config, SSH keys, and workspace naming
- Update tofu.groovy to execute all commands in Docker
- Update ansible.groovy runPlaybook to use Docker
- Pass AWS credentials as environment variables to container
- Mount workspace and SSH keys into container
- Fixes 'tofu: not found' error on Jenkins agents
- Remove invalid steps.env access that caused MissingPropertyException
- AWS credentials now inherited from withCredentials block via -e flags
- Docker run automatically picks up AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY from environment
- Fixes 'No such property: env for class: org.jenkinsci.plugins.workflow.cps.DSL' error
Copy link

@slickwarren slickwarren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not very well versed in this but I don't see anything standing out as an issue. Would like the main contributor's review just in case we miss something.

vars/tofu.groovy Outdated
}

// Run the init-backend.sh script which generates backend.tf and runs tofu init
def initCommand = "cd ${config.dir} && ./scripts/init-backend.sh ${scriptArgs.join(' ')}"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a fan of this being hard-coded, do we do this anywhere else in the shared lib?

what about something like this instead?

if (params?.configureScript) {
// by doing this we accept all build and configure going to run from dir, 'docker build .', currently target is in validation dir so this is necessary
def statusConfigure = steps.sh(script: "./${params.dir}/${params?.configureScript}", returnStatus: true)
if (statusConfigure != 0) {
error "Build script failed with ${statusBuild}"
}
}
def statusBuild = steps.sh(script: "./${params.dir}/${params.buildScript}", returnStatus: true)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored

Copy link

@hamistao hamistao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and looks like will be very helpful. Still I have a couple comments/questions.

// Run ansible-playbook in a container
// [ container: string, dir: string, inventory: string, playbook: string, extraVars?: Map, tags?: string, envVars?: Map ]
def runPlaybookInContainer(Map config) {
if (!(config.container && config.dir && config.inventory && config.playbook)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another nit, if config.container is just the container name, maybe we could generate one and return it or just name it config.container_name. Looks good anyway if you don't want to change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not understanding how this makes it better.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it doesn't, I guess this well within the subjective realm so this is only a suggestion.

My thought was that you could prefer an alternative that has less arguments.

"-w /workspace",
config.container,
"-c",
"\"${ansibleArgs.join(' ')}\""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is, but just to make sure, is it possible that ansibleArgs to contain double quotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a check for that

error 'SSH key content and name must be provided.'
}

def sshDir = config.dir ?: '.ssh'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do a resilient approach like ${HOME}/ssh or something similar that is less dependant on the current directory?

Comment on lines +89 to +93
config.envVars.each { key, value ->
// Replace both ${VAR} and $VAR patterns
processedContent = processedContent.replaceAll(/\$\{${key}\}/, value.toString())
processedContent = processedContent.replaceAll(/\$${key}(?![a-zA-Z0-9_])/, value.toString())
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should process both patterns in write_config as well?


// Archive workspace name for later use
// [ workspaceName: string, fileName?: string ]
def archiveWorkspaceName(Map config) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a curiosity, is there a reason why you are passing all the parameters as a Map config?

Just asking because it seems less straightforward in some cases but perhaps it is a pattern we use that I am not aware of.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a limitation of jenkins / shared libraries IIRC. Something about groovy-java conversions?

Copy link
Member

@caliskanugur caliskanugur Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this example, it can be two string parameters, and the second one can have a default value.

For general Map config:

I haven't tried it fully, but I think the other approach is to know all the types that are being passed to a function -- if that type is needed.

This may introduce a significant curve to writing these functions (especially if our function is calling a DSL or Plugin function), as the types can change with Jenkins and/or plugin versions.

Personally, I don't have a strong opinion on any approach since I used them interchangeably. Trade-off between velocity and static types.

More of a custom function that can be typed vs a function that uses a generic Map instead of the plugin's type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this turns out to be a problem, we can refactor it later.

Copy link
Member

@rancher-max rancher-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Few comments from others -- I commented on a couple from my understanding.


// Archive workspace name for later use
// [ workspaceName: string, fileName?: string ]
def archiveWorkspaceName(Map config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a limitation of jenkins / shared libraries IIRC. Something about groovy-java conversions?

@floatingman
Copy link
Contributor Author

rancher/qa-tasks#1925

@floatingman floatingman self-assigned this Dec 9, 2025
…dling and directory management; enhance generateWorkspaceName to support suffix sanitization
@floatingman floatingman merged commit 9f69d50 into main Dec 11, 2025
@floatingman floatingman deleted the feature/ansible-tofu-jenkins-pipelines branch December 11, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants