feat(ibmcloud): add IBM COS S3-compatible backend support#811
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:
📝 WalkthroughWalkthroughAdds IBM COS (S3-compatible) remote-state support with helpers and env constants, introduces exported DestroyStack/CleanupState lifecycle functions, and wires a new --keep-state flag into IBM Power and IBM Z destroy commands; replaces hardcoded IBM Cloud env lookups with constants. ChangesIBM COS S3-Compatible Backend with Keep-State Flag
🎯 3 (Moderate) | ⏱️ ~25 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
🤖 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/ibmcloud.go`:
- Around line 125-126: The parseS3BackedURL helper can return an empty key and
CleanupState currently calls s3.Delete with that blank key; update
parseS3BackedURL and callers (notably CleanupState and the other block around
the parse use at ~159-167) to validate that the returned key is non-empty before
attempting deletion: either return a descriptive error from parseS3BackedURL
when the URL path yields an empty key, or have CleanupState check the key and
skip/delete only when key != "" (and log/return a clear error if appropriate).
Make sure references to parseS3BackedURL, CleanupState, and s3.Delete are
updated so no Delete is invoked with an empty key.
- Around line 96-113: The code currently calls os.Setenv for AWS_* vars which
mutates process-wide state and can leak/override credentials; instead stop using
os.Setenv in this block and construct the S3 client/config with explicit
credentials and endpoint wiring (e.g. use the AWS SDK config with
credentials.NewStaticCredentialsProvider(accessKey, secretKey, ""), set Region
and a custom EndpointResolver/EndpointResolverWithOptions using
ensureHTTPS(endpoint)) so no environment mutation occurs; update the code that
previously relied on those env vars to accept or receive the aws.Config / s3
client built this way (replace the os.Setenv sequence in
pkg/provider/ibmcloud/ibmcloud.go with creation of an aws.Config/aws-sdk-v2
client using the provided accessKey, secretKey, endpoint, region).
🪄 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: 641f765f-0fde-48d9-b4d1-644371b207d5
📒 Files selected for processing (6)
cmd/mapt/cmd/ibmcloud/hosts/ibm-power.gocmd/mapt/cmd/ibmcloud/hosts/ibm-z.gopkg/provider/ibmcloud/action/ibm-power/ibm-power.gopkg/provider/ibmcloud/action/ibm-z/ibm-z.gopkg/provider/ibmcloud/constants/constants.gopkg/provider/ibmcloud/ibmcloud.go
| package constants | ||
|
|
||
| const ( | ||
| EnvIBMCosAccessKeyID = "IBM_COS_ACCESS_KEY_ID" |
There was a problem hiding this comment.
We already got IBMCLOUD_ACCOUNT and IBMCLOUD_API_KEY and IC_REGION which are used by the natively by the provider, should we keep those I mean use them instead ? Also what is the value for the endpoint?
There was a problem hiding this comment.
Since these are HMAC keys (access key + secret key) and exist specifically for S3-compatible API access, they are separate from IAM API keys and I think we should keep them separate. For region, it should be okay to use IC_REGION.
Re: endpoint - value needs to be set manually right now. Should I look into auto-constructing it?
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/provider/ibmcloud/ibmcloud.go (1)
171-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't report destroy success when state deletion fails.
CleanupStatelogss3.Deleteerrors and still returnsnil. Since the action layer returnsCleanupState(mCtx)directly, a failed delete still exits success even when--keep-stateis false, leaving stale backend state behind.🤖 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/ibmcloud.go` around lines 171 - 175, The CleanupState function currently swallows s3.Delete errors (calling logging.Warnf) and still returns nil, causing callers (e.g., action layer that calls CleanupState(mCtx)) to treat a failed delete as success; update CleanupState so that if s3.Delete(...) returns an error you propagate that error (or wrap it with context) instead of returning nil—refer to the s3.Delete call, the mCtx.Context() usage, and the existing logging.Warnf/Info calls in CleanupState to locate the change, and ensure the success log (logging.Info) only runs when no error is returned.
🤖 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.
Outside diff comments:
In `@pkg/provider/ibmcloud/ibmcloud.go`:
- Around line 171-175: The CleanupState function currently swallows s3.Delete
errors (calling logging.Warnf) and still returns nil, causing callers (e.g.,
action layer that calls CleanupState(mCtx)) to treat a failed delete as success;
update CleanupState so that if s3.Delete(...) returns an error you propagate
that error (or wrap it with context) instead of returning nil—refer to the
s3.Delete call, the mCtx.Context() usage, and the existing logging.Warnf/Info
calls in CleanupState to locate the change, and ensure the success log
(logging.Info) only runs when no error is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5f9f2a0c-59d2-4601-9ceb-91297d542cbc
📒 Files selected for processing (5)
pkg/provider/ibmcloud/constants/constants.gopkg/provider/ibmcloud/data/piimages.gopkg/provider/ibmcloud/data/vpcimages.gopkg/provider/ibmcloud/ibmcloud.gopkg/provider/ibmcloud/services/power/power.go
|
Actionable comments posted: 0 |
|
@deekay2310 can you add sample of usage within the docs for IBM also the squash commits and I think we are good to go |
|
NVM doc is all good, once you tested with last artifacts squash all commits then I will LGTM |
|
The COS Backend connection worked and Pulumi connected to COS successfully. Just going to need to harden how we delete the resources. |
Add provider-specific env vars (IBMCLOUD_COS_ACCESS_KEY_ID, IBMCLOUD_COS_SECRET_ACCESS_KEY, IBMCLOUD_COS_ENDPOINT) and map them to AWS SDK env vars so the Pulumi S3 backend can authenticate to IBM Cloud Object Storage. Construct the full Pulumi backend URL with endpoint query params via Provider.Init() return value, ensuring Pulumi connects to COS instead of amazonaws.com. Also adds DestroyStack with lock cleanup, CleanupState for post-destroy state removal, --keep-state flag, empty key guard in parseS3BackedURL, and consolidates IBMCLOUD_* string constants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Introduce IBM_COS_* env vars (access key, secret, endpoint, region) and remap them to AWS SDK env vars in Init() so the Pulumi S3 backend can authenticate to IBM Cloud Object Storage without conflicting with AWS credentials. Add DestroyStack with lock cleanup, CleanupState for post- destroy state removal, and --keep-state flag to ibm-power/ibm-z destroy commands.