terraform-aws-gitlab-runner
terraform-aws-gitlab-runner copied to clipboard
cattle-ops
fix: always add policy to maintain SSM parameters
Description
According to #509 the Runner always maintains a SSM parameter. Thus it needs the policy to be able to to so.
This MR removes the possibility to steer whether or not to add the policy to allow the Runner to ssm:GetParameter and ssm:PutParameter. The variable enable_manage_gitlab_token is marked as deprecated and no longer evaluated.
Fixes #509
Migrations required
No
Verification
None
Documentation
We use pre-commit to update the Terraform inputs and outputs in the documentation via terraform-docs. Ensure you have installed those components.
Looks good to me, however we should probably make it explicit that the variable now does nothing, not just "deprecated".
Maybe something like:
Ignored and deprecated: This variable used to disable management of the SSM property containing the runner token, meaning that the user-data script still created it, but it wasn't managed by terraform. The runner token is now always stored in a terraform managed SSM property.
As discussed in #509 this MR should probably not be merged. Setting enable_manage_gitlab_token to false allows to reuse the previous runner registration when terraform apply is executed (contrary to registering a new runner in the gitlab server when the value is set to true).
@npalm Can we decide for one way here? Either a) remove the whole functionality as it is not working at the moment and according to your comment in #509 this belongs to an old scenario which is superfluous now or b) fix it.
I propose to remove this function too as the automatic registration process is less error prone.
Not close to a computer this weekend. The automatic registration is the default, right? There where we request gitlab for a token. The other option is legacy for day zero as far I remember.
So suggest we remove the function. I can have a quick look Monday evening
Not 100% sure. This is a quick one solving the problem. The mentioned correct solution could take some time to be completed and could also be a breaking change.
But closing this one would be ok from my side.
@kayman-mk should I test this PR? Can do this next week? Feel free to merge the PR.
For some reason the agent is not staring after applying changes. So blocking the merge for the momet. Sorry
Let me test this as well. Should be running as nothing has been removed from the policies.
@npalm This branch is running in my environment. Deployed a runner and executed one job successfully.
@npalm This branch is running in my environment. Deployed a runner and executed one job successfully.
Will run a test asap. Assume it is now fine. Thanks!