Skip to content

Conversation

sanchezl
Copy link
Contributor

No description provided.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 10, 2025
Copy link
Contributor

openshift-ci bot commented Sep 10, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link

coderabbitai bot commented Sep 10, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

openshift-ci bot commented Sep 10, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sanchezl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2025
@sanchezl
Copy link
Contributor Author

/test e2e-gcp-operator

Copy link
Contributor

@benluddy benluddy left a comment

Choose a reason for hiding this comment

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

Is this fixing an issue? It'd be ideal to have a regression test alongside (I see the PR is marked WIP but I thought I'd ask to better understand the context myself).

Comment on lines +417 to 423
if (result != nil && result.ResourceVersion == "1") || creationRequired {
// either the configmap was created or we expected it to be created and it failed
resourcehelper.ReportCreateEvent(recorder, caBundleConfigMap, err)
} else {
// an existing configmap was updated, even if we expected it to be created
resourcehelper.ReportUpdateEvent(recorder, caBundleConfigMap, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC apply responses will have HTTP status 201 if it caused the resource to be created and 200 otherwise. That would be 100% correct for cases where you get a response vs. a prediction based on the cached state.

Comment on lines +408 to +410
tmp := &metav1.ObjectMeta{}
additionalAnnotations.EnsureTLSMetadataUpdate(tmp)
patch.WithAnnotations(tmp.Annotations)
Copy link
Contributor

Choose a reason for hiding this comment

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

We're likely to make a mistake eventually if we need to keep this in sync with CombineCABundleConfigMapsOptimistically. Can we build the apply configuration purely from requiredConfigMap?

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 agree. I would image we would refactor the helpers to be a more appropriate, but for this POC just 'hacked' it in.

Copy link
Contributor

openshift-ci bot commented Sep 10, 2025

@sanchezl: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants