feat(ibmcloud): add GitLab runner support for IBM Power (ppc64le) and IBM Z (s390x)#812
Open
adrianriobo wants to merge 2 commits into
Open
feat(ibmcloud): add GitLab runner support for IBM Power (ppc64le) and IBM Z (s390x)#812adrianriobo wants to merge 2 commits into
adrianriobo wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR extends the mapt CLI and infrastructure provisioning for IBM PowerVS and IBM Z cloud instances to support optional GitLab runner deployment. It adds new architectures, parameter builders, command wiring, cloud-init templates, and tests covering runner-only and runner+observability scenarios. ChangesGitLab Runner Integration for IBM Cloud
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cmd/mapt/cmd/params/params.go (1)
288-371: ⚡ Quick winDeduplicate GitLab runner arg builders.
GitLabRunnerArgs,IBMPowerGitLabRunnerArgs, andIBMZGitLabRunnerArgsare identical except for theArchfield. Consider extracting a single helper that accepts the target*gitlab.Archto avoid drift if fields are added later.♻️ Proposed refactor
-func GitLabRunnerArgs() *gitlab.GitLabRunnerArgs { - if viper.IsSet(glRunnerToken) { - return &gitlab.GitLabRunnerArgs{ - GitLabPAT: viper.GetString(glRunnerToken), - ProjectID: viper.GetString(glRunnerProjectID), - GroupID: viper.GetString(glRunnerGroupID), - URL: viper.GetString(glRunnerURL), - Tags: viper.GetStringSlice(glRunnerTags), - Platform: &gitlab.Linux, - Arch: linuxArchAsGitLabArch(viper.GetString(LinuxArch)), - } - } - return nil -} +func GitLabRunnerArgs() *gitlab.GitLabRunnerArgs { + return gitLabRunnerArgsFor(linuxArchAsGitLabArch(viper.GetString(LinuxArch))) +} + +func IBMPowerGitLabRunnerArgs() *gitlab.GitLabRunnerArgs { + return gitLabRunnerArgsFor(&gitlab.Ppc64le) +} + +func IBMZGitLabRunnerArgs() *gitlab.GitLabRunnerArgs { + return gitLabRunnerArgsFor(&gitlab.S390x) +} + +func gitLabRunnerArgsFor(arch *gitlab.Arch) *gitlab.GitLabRunnerArgs { + if !viper.IsSet(glRunnerToken) { + return nil + } + return &gitlab.GitLabRunnerArgs{ + GitLabPAT: viper.GetString(glRunnerToken), + ProjectID: viper.GetString(glRunnerProjectID), + GroupID: viper.GetString(glRunnerGroupID), + URL: viper.GetString(glRunnerURL), + Tags: viper.GetStringSlice(glRunnerTags), + Platform: &gitlab.Linux, + Arch: arch, + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/mapt/cmd/params/params.go` around lines 288 - 371, The three functions GitLabRunnerArgs, IBMPowerGitLabRunnerArgs, and IBMZGitLabRunnerArgs are duplicated except for the Arch field; create a single helper (e.g., makeGitLabRunnerArgsWithArch or gitLabRunnerArgsWithArch) that accepts an *gitlab.Arch and returns *gitlab.GitLabRunnerArgs, move the shared construction (GitLabPAT, ProjectID, GroupID, URL, Tags, Platform) into that helper while keeping the viper.IsSet(glRunnerToken) check, then refactor GitLabRunnerArgs, IBMPowerGitLabRunnerArgs and IBMZGitLabRunnerArgs to call this helper with linuxArchAsGitLabArch(...), &gitlab.Ppc64le, and &gitlab.S390x respectively.pkg/provider/ibmcloud/action/ibm-power/cloud-config (1)
155-159: ⚖️ Poor tradeoffImplicit indentation contract on
GitLabRunnerScript.
{{.GitLabRunnerScript}}is interpolated at column 0 directly undercontent: |, so the caller must pre-indent every line of the script to the YAML block's required indent (≥6 spaces, as the test fixture demonstrates). Any caller that forgets — or a script whose first line lacks indentation — produces malformed cloud-config that may parse silently as truncated content. Consider using a template helper (e.g., a smallindentfunction passed tofile.Template) so the template controls indentation explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/provider/ibmcloud/action/ibm-power/cloud-config` around lines 155 - 159, The template interpolates .GitLabRunnerScript at column 0 under the YAML block, forcing callers to pre-indent lines; change the cloud-config template to apply an indentation helper so the template, not callers, controls indentation. Add or wire a small helper function (e.g., indent) into the template funcs used by file.Template and use it when rendering the content block (reference .GitLabRunnerScript and the content: | block) so you render something like {{ indent .GitLabRunnerScript 6 }} to ensure every script line is correctly indented and prevent malformed YAML.pkg/provider/ibmcloud/action/ibm-z/cloud-config (1)
155-159: 💤 Low valueTemplate requires the caller to pre-indent
GitLabRunnerScript.
{{.GitLabRunnerScript}}is interpolated directly into a YAMLcontent: |block, so the script must already be indented (the tests use 6 spaces). Go'stext/templatewill not adjust indentation, so a non-indented script will silently produce malformed YAML and cloud-init will fail to parse the file. Worth either documenting the indentation contract at the call site (integrations.GetIntegrationSnippetAsCloudInitWritableFile) or using{{ .GitLabRunnerScript | indent 6 }}-style helper if your template engine supports it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/provider/ibmcloud/action/ibm-z/cloud-config` around lines 155 - 159, The cloud-config template inserts {{.GitLabRunnerScript}} directly into a YAML literal block under content: | which requires the script to be pre-indented; update the template (symbol: GitLabRunnerScript in the cloud-config template) to ensure proper indentation by either piping the value through an indent helper (e.g., use an indent filter like "{{ .GitLabRunnerScript | indent 6 }}" if the template engine supports it) or document/ enforce the indentation contract at the caller (integrations.GetIntegrationSnippetAsCloudInitWritableFile) so callers always supply a 6-space indented script; make the change in the cloud-config template (or the caller) so cloud-init receives valid YAML.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/provider/ibmcloud/action/ibm-z/cloud-config`:
- Around line 161-162: The cloud-init runcmd currently runs "apt-get install -y
git podman" which can fail on fresh images because the package index isn't
updated; update the package index first or enable cloud-init's package_update:
true so the installers succeed. Modify the cloud-init snippet that contains the
runcmd entry (the line with "apt-get install -y git podman") to either prepend
an "apt-get update" command (e.g., run "apt-get update" before the install) or
set "package_update: true" at the top of the config so that package lists are
refreshed before the install step.
In `@pkg/provider/ibmcloud/action/ibm-z/ibm-z.go`:
- Around line 355-361: In buildUserDataInput's synchronous hasOtel branch, do
not swallow errors from izUserData; instead propagate the error to callers (or
surface it as a Pulumi output) so deployments fail when OTEL setup fails. Modify
the hasOtel branch that currently does "ud, err := izUserData(...); if err !=
nil { return nil }" to return the error (or a failing Pulumi output) and update
all callers (deploy and deployWithExistingSubnet) to accept and handle the
propagated error (or passthrough the Pulumi error output) so OTEL failures are
not silently ignored.
---
Nitpick comments:
In `@cmd/mapt/cmd/params/params.go`:
- Around line 288-371: The three functions GitLabRunnerArgs,
IBMPowerGitLabRunnerArgs, and IBMZGitLabRunnerArgs are duplicated except for the
Arch field; create a single helper (e.g., makeGitLabRunnerArgsWithArch or
gitLabRunnerArgsWithArch) that accepts an *gitlab.Arch and returns
*gitlab.GitLabRunnerArgs, move the shared construction (GitLabPAT, ProjectID,
GroupID, URL, Tags, Platform) into that helper while keeping the
viper.IsSet(glRunnerToken) check, then refactor GitLabRunnerArgs,
IBMPowerGitLabRunnerArgs and IBMZGitLabRunnerArgs to call this helper with
linuxArchAsGitLabArch(...), &gitlab.Ppc64le, and &gitlab.S390x respectively.
In `@pkg/provider/ibmcloud/action/ibm-power/cloud-config`:
- Around line 155-159: The template interpolates .GitLabRunnerScript at column 0
under the YAML block, forcing callers to pre-indent lines; change the
cloud-config template to apply an indentation helper so the template, not
callers, controls indentation. Add or wire a small helper function (e.g.,
indent) into the template funcs used by file.Template and use it when rendering
the content block (reference .GitLabRunnerScript and the content: | block) so
you render something like {{ indent .GitLabRunnerScript 6 }} to ensure every
script line is correctly indented and prevent malformed YAML.
In `@pkg/provider/ibmcloud/action/ibm-z/cloud-config`:
- Around line 155-159: The cloud-config template inserts {{.GitLabRunnerScript}}
directly into a YAML literal block under content: | which requires the script to
be pre-indented; update the template (symbol: GitLabRunnerScript in the
cloud-config template) to ensure proper indentation by either piping the value
through an indent helper (e.g., use an indent filter like "{{
.GitLabRunnerScript | indent 6 }}" if the template engine supports it) or
document/ enforce the indentation contract at the caller
(integrations.GetIntegrationSnippetAsCloudInitWritableFile) so callers always
supply a 6-space indented script; make the change in the cloud-config template
(or the caller) so cloud-init receives valid YAML.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: f0e92b56-6ebf-4360-be17-a832ebfe2b75
📒 Files selected for processing (11)
cmd/mapt/cmd/ibmcloud/hosts/ibm-power.gocmd/mapt/cmd/ibmcloud/hosts/ibm-z.gocmd/mapt/cmd/params/params.gopkg/integrations/gitlab/snippet-linux.shpkg/integrations/gitlab/types.gopkg/provider/ibmcloud/action/ibm-power/cloud-configpkg/provider/ibmcloud/action/ibm-power/ibm-power.gopkg/provider/ibmcloud/action/ibm-power/ibm-power_test.gopkg/provider/ibmcloud/action/ibm-z/cloud-configpkg/provider/ibmcloud/action/ibm-z/ibm-z.gopkg/provider/ibmcloud/action/ibm-z/ibm-z_test.go
adrianriobo
force-pushed
the
ibm-gitlab-support
branch
from
2ab0a4d to
b186b5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
pkg/provider/ibmcloud/action/ibm-z/ibm-z.go (1)
355-359:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate OTEL user-data render failures.
Line 358 still drops the
izUserDataerror and returnsnil, so the VM can be created without OTEL config while the deployment appears successful.Suggested fix
-func (r *zRequest) buildUserDataInput() pulumi.StringPtrInput { +func (r *zRequest) buildUserDataInput() (pulumi.StringPtrInput, error) { hasOtel := r.otelAppCode != "" && r.otelAuthToken != "" if r.glAuthToken != nil { - return r.glAuthToken.ApplyT(func(token string) (*string, error) { + return r.glAuthToken.ApplyT(func(token string) (*string, error) { ... - }).(pulumi.StringPtrOutput) + }).(pulumi.StringPtrOutput), nil } if hasOtel { ud, err := izUserData(r.otelAppCode, r.otelAuthToken, r.otelEndpoint, r.otelIndex, "", r.otelExtraAttrs) if err != nil { - return nil + return nil, err } - return pulumi.StringPtr(ud) + return pulumi.StringPtr(ud), nil } - return nil + return nil, nil }And update both callers before assigning
instanceArgs.UserData/existingSubnetInstanceArgs.UserData.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/provider/ibmcloud/action/ibm-z/ibm-z.go` around lines 355 - 359, The izUserData call is swallowing errors (returning nil on error) which allows VMs to be created without OTEL config; change the logic in the code that calls izUserData (the call in ibm-z.go that assigns instanceArgs.UserData and existingSubnetInstanceArgs.UserData) to propagate the returned error instead of returning nil—i.e., check the error from izUserData, return that error to the caller if non-nil, and only assign instanceArgs.UserData / existingSubnetInstanceArgs.UserData when izUserData succeeds; also update both callers to handle and return the propagated error rather than proceeding with a nil user-data.pkg/provider/ibmcloud/action/ibm-z/cloud-config (1)
161-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh APT metadata before installing packages.
On a fresh image, Line 162 can fail with
Unable to locate package, which aborts the rest ofruncmdand leaves the runner/OTEL setup incomplete.Suggested fix
runcmd: + - apt-get update - apt-get install -y git podman🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/provider/ibmcloud/action/ibm-z/cloud-config` around lines 161 - 163, In the cloud-config runcmd block, the line that runs "apt-get install -y git podman" can fail on fresh images because APT metadata isn't refreshed; update the package index before installing by inserting an "apt-get update" (or "apt-get update -y" variant) immediately before the existing "apt-get install -y git podman" command in the runcmd list so the install runs against fresh package metadata; modify the runcmd sequence around the "apt-get install -y git podman" entry to run the update first (e.g., add an "apt-get update" item above that install).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/mapt/cmd/params/params.go`:
- Around line 343-371: Both IBMPowerGitLabRunnerArgs and IBMZGitLabRunnerArgs
currently forward glRunnerProjectID and glRunnerGroupID even though they are
mutually exclusive; add a CLI-side validation in each function that checks
viper.IsSet(glRunnerProjectID) && viper.IsSet(glRunnerGroupID) and if both are
set abort immediately (e.g., log a clear error via the existing logger or fmt
and call os.Exit(1) or return nil after signaling the error) before constructing
and returning the GitLabRunnerArgs so a hosts provision step cannot proceed with
invalid mutually-exclusive options.
In `@pkg/provider/ibmcloud/action/ibm-power/cloud-config`:
- Around line 149-154: The runner service's logging conf appends all output to
/var/log/gitlab-runner/runner.log without rotation; add a log rotation policy
(e.g., create a /etc/logrotate.d/gitlab-runner entry) to rotate
/var/log/gitlab-runner/runner.log regularly, compress/rotate old files, and set
retention and permissions to prevent disk exhaustion; ensure this new logrotate
resource is included alongside the existing change that creates
/etc/systemd/system/gitlab-runner.service.d/logging.conf (and apply the same fix
for the other occurrence that appends to runner.log).
In `@pkg/provider/ibmcloud/action/ibm-power/ibm-power.go`:
- Around line 174-177: The code uses authToken.ApplyT to run a closure that
calls gitlab.SetAuthToken and reads global state via gitlab.GetRunnerArgs when
building piUserDataInput, which can mutate global runner args and leak tokens
across concurrent runs; refactor so the ApplyT closure only uses immutable
inputs by capturing a copy of runner args and any needed auth values outside the
closure (e.g., call gitlab.GetRunnerArgs() and obtain the token beforehand),
then pass those values into the function given to authToken.ApplyT and remove
calls to gitlab.SetAuthToken inside the closure; ensure
integrations.GetIntegrationSnippetAsCloudInitWritableFile and defaultUser are
invoked with local copies so no global gitlab state is mutated during async
userdata generation.
In `@pkg/provider/ibmcloud/action/ibm-z/cloud-config`:
- Around line 76-94: The filelog receiver configuration filelog/gitlab-runner
currently uses start_at: beginning without any filelog persistent
storage/extensions, which causes full runner.log replay on otelcol restarts;
either add a persistent storage extension (e.g., configure extensions/storage or
file_storage checkpointing) for that receiver or change start_at to end to
prevent replay, and ensure the operators and attributes (filelog/gitlab-runner,
include: /var/log/gitlab-runner/runner.log, operators
move_to_source_name/remove_file_name) remain unchanged; additionally, in the
bootstrap runcmd that runs apt-get install -y git podman, prepend an apt-get
update step to make package installation reliable on fresh images.
---
Duplicate comments:
In `@pkg/provider/ibmcloud/action/ibm-z/cloud-config`:
- Around line 161-163: In the cloud-config runcmd block, the line that runs
"apt-get install -y git podman" can fail on fresh images because APT metadata
isn't refreshed; update the package index before installing by inserting an
"apt-get update" (or "apt-get update -y" variant) immediately before the
existing "apt-get install -y git podman" command in the runcmd list so the
install runs against fresh package metadata; modify the runcmd sequence around
the "apt-get install -y git podman" entry to run the update first (e.g., add an
"apt-get update" item above that install).
In `@pkg/provider/ibmcloud/action/ibm-z/ibm-z.go`:
- Around line 355-359: The izUserData call is swallowing errors (returning nil
on error) which allows VMs to be created without OTEL config; change the logic
in the code that calls izUserData (the call in ibm-z.go that assigns
instanceArgs.UserData and existingSubnetInstanceArgs.UserData) to propagate the
returned error instead of returning nil—i.e., check the error from izUserData,
return that error to the caller if non-nil, and only assign
instanceArgs.UserData / existingSubnetInstanceArgs.UserData when izUserData
succeeds; also update both callers to handle and return the propagated error
rather than proceeding with a nil user-data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: cde08b39-3023-4370-a54e-3c7ee8b2140e
📒 Files selected for processing (12)
cmd/mapt/cmd/ibmcloud/hosts/ibm-power.gocmd/mapt/cmd/ibmcloud/hosts/ibm-z.gocmd/mapt/cmd/params/params.gopkg/integrations/gitlab/glrunner.gopkg/integrations/gitlab/snippet-linux.shpkg/integrations/gitlab/types.gopkg/provider/ibmcloud/action/ibm-power/cloud-configpkg/provider/ibmcloud/action/ibm-power/ibm-power.gopkg/provider/ibmcloud/action/ibm-power/ibm-power_test.gopkg/provider/ibmcloud/action/ibm-z/cloud-configpkg/provider/ibmcloud/action/ibm-z/ibm-z.gopkg/provider/ibmcloud/action/ibm-z/ibm-z_test.go
adrianriobo
force-pushed
the
ibm-gitlab-support
branch
from
b186b5b to
ecb417d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cmd/mapt/cmd/params/params.go (1)
291-294:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast instead of silently disabling GitLab runner setup.
At Line 291–Line 294, invalid mutually-exclusive flags only log and return
nil, so host provisioning can continue without runner registration. This is a costly silent failure path; the command should abort with an explicit error when both flags are set.As per coding guidelines "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/mapt/cmd/params/params.go` around lines 291 - 294, The code currently treats mutually-exclusive flags glRunnerProjectID and glRunnerGroupID by logging via logging.Error and returning nil, which silently allows provisioning to continue; change this to fail fast by returning an explicit error (e.g., fmt.Errorf or errors.New) instead of nil so the calling command aborts; update the block where logging.Error is called (the mutual-exclusion check) to produce and return a descriptive error (including the flag names) rather than just logging, ensuring the function signature supports an error return.
🧹 Nitpick comments (1)
pkg/provider/ibmcloud/action/ibm-power/ibm-power_test.go (1)
52-81: ⚡ Quick winAdd coverage for the logrotate stanza in runner-enabled cloud-config.
The test validates log redirection, but not the rotation policy. A small assertion here protects against regression of the disk-growth safeguard.
Proposed test addition
func TestPiUserData_withOtelAndRunner(t *testing.T) { @@ if !strings.Contains(cfg, "StandardOutput=append:/var/log/gitlab-runner") { t.Error("expected systemd drop-in to redirect runner stdout to log file") } + if !strings.Contains(cfg, "/etc/logrotate.d/gitlab-runner") || + !strings.Contains(cfg, "rotate 7") { + t.Error("expected logrotate policy for gitlab-runner logs") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/provider/ibmcloud/action/ibm-power/ibm-power_test.go` around lines 52 - 81, The test TestPiUserData_withOtelAndRunner checks log redirection but misses asserting the logrotate stanza; update this test to assert that the generated cloud-config (decoded cfg from piUserData) contains the runner logrotate configuration (e.g., presence of a logrotate file/path or directives such as a gitlab-runner logrotate stanza like a filename under /etc/logrotate.d or rotation directives like "rotate" or "size" for /var/log/gitlab-runner/runner.log). Modify TestPiUserData_withOtelAndRunner to include one concise assertion that strings.Contains(cfg, "<expected logrotate snippet>") referencing piUserData and the test name so future regressions of the rotation policy are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/mapt/cmd/ibmcloud/hosts/ibm-power.go`:
- Line 56: The call sites are using a non-existent function
params.GitLabRunnerArgsForArch; replace those calls with the existing
params.GitLabRunnerArgs by passing the architecture pointer directly (e.g.,
params.GitLabRunnerArgs(&gitlab.Ppc64le) in ibm-power.go and the analogous
&gitlab.<arch> in ibm-z.go), or alternatively add a small wrapper function named
GitLabRunnerArgsForArch in the params package that simply forwards to
GitLabRunnerArgs; update references to use the real symbol (GitLabRunnerArgs or
the new wrapper) so the code compiles.
In `@pkg/integrations/gitlab/snippet-linux.sh`:
- Around line 11-12: The restorecon invocation in snippet-linux.sh currently
swallows all errors via "|| true"; change it so real SELinux relabel failures
are not suppressed by first checking that restorecon exists and SELinux is
enabled (e.g., via command -v restorecon and a check like getenforce/sestatus)
before running restorecon -v /usr/bin/gitlab-runner; only ignore failures when
the tool or SELinux policy is absent/unavailable, otherwise let the command fail
(or emit an explicit error) so labeling problems are visible and can be handled.
In `@pkg/provider/ibmcloud/action/ibm-z/cloud-config`:
- Around line 167-170: The installer script at path "/opt/install-glrunner.sh"
is written with permissions '0755' exposing the injected GitLabRunnerScript (and
its auth token) to other local users; update the resource that writes
GitLabRunnerScript so the permissions are more restrictive (e.g., '0700' or
'0600') and, if appropriate, add a step to delete or shred
/opt/install-glrunner.sh after execution to avoid leaving credentials on disk;
locate the block that sets path: /opt/install-glrunner.sh and change the
permissions field and/or add cleanup logic tied to the installer execution.
In `@pkg/provider/ibmcloud/action/ibm-z/ibm-z.go`:
- Around line 344-373: The OTEL enable check in buildUserDataInput is too lax:
hasOtel only verifies r.otelAppCode and r.otelAuthToken but izUserData also
requires r.otelEndpoint and r.otelIndex; this can produce invalid user-data or
silently skip setup. Update the condition to require a complete OTEL config
(check r.otelAppCode, r.otelAuthToken, r.otelEndpoint and r.otelIndex) before
calling izUserData or setting otelCode/otelTok inside the glAuthToken.ApplyT
block, and return an error when required OTEL fields are partially present;
ensure all places that call izUserData only pass when the full set is present.
---
Duplicate comments:
In `@cmd/mapt/cmd/params/params.go`:
- Around line 291-294: The code currently treats mutually-exclusive flags
glRunnerProjectID and glRunnerGroupID by logging via logging.Error and returning
nil, which silently allows provisioning to continue; change this to fail fast by
returning an explicit error (e.g., fmt.Errorf or errors.New) instead of nil so
the calling command aborts; update the block where logging.Error is called (the
mutual-exclusion check) to produce and return a descriptive error (including the
flag names) rather than just logging, ensuring the function signature supports
an error return.
---
Nitpick comments:
In `@pkg/provider/ibmcloud/action/ibm-power/ibm-power_test.go`:
- Around line 52-81: The test TestPiUserData_withOtelAndRunner checks log
redirection but misses asserting the logrotate stanza; update this test to
assert that the generated cloud-config (decoded cfg from piUserData) contains
the runner logrotate configuration (e.g., presence of a logrotate file/path or
directives such as a gitlab-runner logrotate stanza like a filename under
/etc/logrotate.d or rotation directives like "rotate" or "size" for
/var/log/gitlab-runner/runner.log). Modify TestPiUserData_withOtelAndRunner to
include one concise assertion that strings.Contains(cfg, "<expected logrotate
snippet>") referencing piUserData and the test name so future regressions of the
rotation policy are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: df5ecc4c-6385-47e1-a8d9-591783baf011
📒 Files selected for processing (12)
cmd/mapt/cmd/ibmcloud/hosts/ibm-power.gocmd/mapt/cmd/ibmcloud/hosts/ibm-z.gocmd/mapt/cmd/params/params.gopkg/integrations/gitlab/glrunner.gopkg/integrations/gitlab/snippet-linux.shpkg/integrations/gitlab/types.gopkg/provider/ibmcloud/action/ibm-power/cloud-configpkg/provider/ibmcloud/action/ibm-power/ibm-power.gopkg/provider/ibmcloud/action/ibm-power/ibm-power_test.gopkg/provider/ibmcloud/action/ibm-z/cloud-configpkg/provider/ibmcloud/action/ibm-z/ibm-z.gopkg/provider/ibmcloud/action/ibm-z/ibm-z_test.go
Comment on lines
+11
to
+12
| # Fix SELinux context (no-op on non-SELinux systems) | ||
| sudo restorecon -v /usr/bin/gitlab-runner 2>/dev/null || true |
There was a problem hiding this comment.
Don’t suppress real SELinux relabel failures on SELinux-enabled hosts.
At Line 12, || true makes all restorecon failures non-fatal, including real labeling problems that can break runner execution later. Keep it non-fatal only when SELinux tooling/policy is unavailable.
Suggested hardening
-# Fix SELinux context (no-op on non-SELinux systems)
-sudo restorecon -v /usr/bin/gitlab-runner 2>/dev/null || true
+# Fix SELinux context (no-op when SELinux is not enabled)
+if command -v restorecon >/dev/null 2>&1 && command -v selinuxenabled >/dev/null 2>&1 && selinuxenabled; then
+ sudo restorecon -v /usr/bin/gitlab-runner
+fiAs per coding guidelines "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Fix SELinux context (no-op on non-SELinux systems) | |
| sudo restorecon -v /usr/bin/gitlab-runner 2>/dev/null || true | |
| # Fix SELinux context (no-op when SELinux is not enabled) | |
| if command -v restorecon >/dev/null 2>&1 && command -v selinuxenabled >/dev/null 2>&1 && selinuxenabled; then | |
| sudo restorecon -v /usr/bin/gitlab-runner | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/integrations/gitlab/snippet-linux.sh` around lines 11 - 12, The
restorecon invocation in snippet-linux.sh currently swallows all errors via "||
true"; change it so real SELinux relabel failures are not suppressed by first
checking that restorecon exists and SELinux is enabled (e.g., via command -v
restorecon and a check like getenforce/sestatus) before running restorecon -v
/usr/bin/gitlab-runner; only ignore failures when the tool or SELinux policy is
absent/unavailable, otherwise let the command fail (or emit an explicit error)
so labeling problems are visible and can be handled.
e34e1fb to
d465c2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/provider/ibmcloud/action/ibm-z/cloud-config (1)
167-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict
/opt/install-glrunner.shpermissions to root-only.This script can contain runner credentials, so
0755exposes sensitive content to non-root local users.Suggested fix
- path: /opt/install-glrunner.sh - permissions: '0755' + permissions: '0700' content: | {{.GitLabRunnerScript}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/provider/ibmcloud/action/ibm-z/cloud-config` around lines 167 - 170, The cloud-config entry currently sets the installer script at path "/opt/install-glrunner.sh" with permissions '0755', exposing sensitive runner credentials to non-root users; update the permissions value for that resource to a root-only mode (e.g., '0700') so only root can read/execute the script, leaving the content ({{.GitLabRunnerScript}}) and path unchanged; locate the YAML block that defines path: /opt/install-glrunner.sh and change the permissions field accordingly.pkg/provider/ibmcloud/action/ibm-z/ibm-z.go (1)
345-367:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
hasOtelcheck is still incomplete (endpoint/index not enforced).The OTEL gate only checks app code + auth token. Missing endpoint or index can still produce silent misconfiguration. Require all four OTEL fields together, and error on partial config.
Suggested fix
func (r *zRequest) buildUserDataInput() (pulumi.StringPtrInput, error) { - hasOtel := r.otelAppCode != "" && r.otelAuthToken != "" + otelConfigured := r.otelAppCode != "" || r.otelAuthToken != "" || r.otelEndpoint != "" || r.otelIndex != "" + if otelConfigured && + (r.otelAppCode == "" || r.otelAuthToken == "" || r.otelEndpoint == "" || r.otelIndex == "") { + return nil, fmt.Errorf("otel configuration requires app code, auth token, endpoint, and index") + } + hasOtel := otelConfigured🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/provider/ibmcloud/action/ibm-z/ibm-z.go` around lines 345 - 367, The OTEL gating currently only checks r.otelAppCode and r.otelAuthToken; change the logic so hasOtel means all four fields are present (r.otelAppCode, r.otelAuthToken, r.otelEndpoint, r.otelIndex are non-empty). Add a validation path that detects any partial OTEL config (some fields set but not all) and return an error (from both the r.glAuthToken.ApplyT block and the non-ApplyT branch) instead of proceeding. Update the local checks and calls to izUserData (and the returned ud) to rely on the new hasOtel boolean and ensure the error is returned when partial config is detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/provider/ibmcloud/action/ibm-power/cloud-config`:
- Around line 167-170: The cloud-config currently creates
/opt/install-glrunner.sh with permissions '0755' which may expose the injected
runner auth token; update the resource that sets path: /opt/install-glrunner.sh
to use stricter permissions (e.g., '0700' or '0600') and ensure ownership is
root (add owner: root:root or equivalent) so only root can read/execute the
installer script; change the permissions field and add an owner field in the
same cloud-config entry to lock down the file.
In `@pkg/provider/ibmcloud/action/ibm-power/ibm-power.go`:
- Around line 166-193: Before calling piUserData ensure the OTEL fields are
either all provided or all empty: validate r.otelAppCode, r.otelAuthToken,
r.otelEndpoint and r.otelIndex together and return an error if some are set and
others are missing. Apply this validation in both places where piUserData is
invoked (inside the gitlab CreateRunner ApplyT block that sets piUserDataInput
and in the else branch that sets piUserDataInput directly) so piUserData never
receives a partially-complete OTEL config.
---
Duplicate comments:
In `@pkg/provider/ibmcloud/action/ibm-z/cloud-config`:
- Around line 167-170: The cloud-config entry currently sets the installer
script at path "/opt/install-glrunner.sh" with permissions '0755', exposing
sensitive runner credentials to non-root users; update the permissions value for
that resource to a root-only mode (e.g., '0700') so only root can read/execute
the script, leaving the content ({{.GitLabRunnerScript}}) and path unchanged;
locate the YAML block that defines path: /opt/install-glrunner.sh and change the
permissions field accordingly.
In `@pkg/provider/ibmcloud/action/ibm-z/ibm-z.go`:
- Around line 345-367: The OTEL gating currently only checks r.otelAppCode and
r.otelAuthToken; change the logic so hasOtel means all four fields are present
(r.otelAppCode, r.otelAuthToken, r.otelEndpoint, r.otelIndex are non-empty). Add
a validation path that detects any partial OTEL config (some fields set but not
all) and return an error (from both the r.glAuthToken.ApplyT block and the
non-ApplyT branch) instead of proceeding. Update the local checks and calls to
izUserData (and the returned ud) to rely on the new hasOtel boolean and ensure
the error is returned when partial config is detected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 03dd32f1-c3cd-4640-8dc3-38d13fd45295
📒 Files selected for processing (19)
cmd/mapt/cmd/aws/hosts/fedora.gocmd/mapt/cmd/aws/hosts/mac.gocmd/mapt/cmd/aws/hosts/rhel.gocmd/mapt/cmd/aws/hosts/windows.gocmd/mapt/cmd/aws/services/mac-pool.gocmd/mapt/cmd/azure/hosts/rhel.gocmd/mapt/cmd/azure/hosts/windows.gocmd/mapt/cmd/ibmcloud/hosts/ibm-power.gocmd/mapt/cmd/ibmcloud/hosts/ibm-z.gocmd/mapt/cmd/params/params.gopkg/integrations/gitlab/glrunner.gopkg/integrations/gitlab/snippet-linux.shpkg/integrations/gitlab/types.gopkg/provider/ibmcloud/action/ibm-power/cloud-configpkg/provider/ibmcloud/action/ibm-power/ibm-power.gopkg/provider/ibmcloud/action/ibm-power/ibm-power_test.gopkg/provider/ibmcloud/action/ibm-z/cloud-configpkg/provider/ibmcloud/action/ibm-z/ibm-z.gopkg/provider/ibmcloud/action/ibm-z/ibm-z_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/mapt/cmd/aws/hosts/rhel.go
adrianriobo
force-pushed
the
ibm-gitlab-support
branch
21 times, most recently
from
997872b to
8cfca01
Compare
adrianriobo
force-pushed
the
ibm-gitlab-support
branch
4 times, most recently
from
5eefb0d to
588d851
Compare
- Register GitLab project or group runners via Pulumi NewUserRunner API during stack provisioning; auth token flows through ApplyT to avoid global state mutation - Add --glrunner-unsecure flag (default false): by default a locked-down gitlab-runner system user is created with rootless-Podman support (subuid/subgid + loginctl enable-linger); when true the runner runs as the default OS user - Create gitlab-runner system user with /bin/bash shell so PAM does not reject su calls from the shell executor (nologin caused immediate 'prepare environment: exit status 1') - Install and configure otelcol-contrib filelog collector for syslog, secure/auth, audit, and gitlab-runner log shipping via OTLP HTTP; use apt-get/dnf for installation to resolve package dependencies automatically - Exclude otel-endpoint from partial-config validation since it carries a default value; only validate app-code, auth-token, and index - Upgrade pulumi-gitlab SDK from v8 to v9 to match Containerfile pre-installed plugin (v9.11.0); bump pulumi-tls to v5.5.0 - Add systemd drop-in + logrotate for /var/log/gitlab-runner/runner.log - IBM Power: use dnf for otelcol-contrib install; set route via VPC gateway; deploy optional VPC bastion with floating IP for SSH access - IBM Z: wrap cloud-config in MIME multipart with base64 encoding so cloud-init decodes the payload before processing - Update all module dependencies Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…module Move otelcol-contrib install/config out of IBM-specific cloud-configs into pkg/integrations/otelcol. The module uses a binary tarball download instead of apt-get/dnf so it works on any Linux distro without package-manager dependency conflicts (avoids dpkg conffile prompt when config.yaml is pre-written by cloud-init write_files before the package installs). - New pkg/integrations/otelcol with types, snippet-linux.sh template, and GetSnippet/GetSnippetAsCloudInitWritableFile helpers - IBM Z and IBM Power cloud-configs simplified to write/run the otelcol install script, removing all inline otelcol YAML - Makefile ldflags consolidated to single otelcol.version override - Tests updated to match new piUserData/izUserData signatures Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
adrianriobo
force-pushed
the
ibm-gitlab-support
branch
from
588d851 to
26e2503
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ppc64le(IBM Power VS) ands390x(IBM Z) arch types--glrunner-*flags into theibm-powerandibm-zcreate commands; arch is inferred from the target, no extra flag needed/var/log/gitlab-runner/runner.logvia a systemd drop-in — ensuring job logs are audited locally, independent of GitLab connectivityfilelog/gitlab-runnerreceiver is added to the otelcol pipeline so runner job traces are shipped to the OTLP endpoint as part of the audit trailTest plan
go test ./pkg/provider/ibmcloud/action/ibm-power/... ./pkg/provider/ibmcloud/action/ibm-z/... -vpassesgo build ./cmd/maptcompiles cleanlymapt ibmcloud ibm-power create --glrunner-token <PAT> --glrunner-project-id <ID> ...provisions machine with runner registeredmapt ibmcloud ibm-z create --glrunner-token <PAT> --glrunner-project-id <ID> ...provisions machine with runner registered/var/log/gitlab-runner/runner.logis present on the machine and logs are visible in the OTLP backend🤖 Generated with Claude Code