Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OSPRH-11235] Do not fail when clouds CM exists #240

Conversation

lpiwowar
Copy link
Collaborator

@lpiwowar lpiwowar commented Nov 5, 2024

The Ensure[Horizon|Tobiko]CloudsYAML function was failing when the CM containing the modified clouds.yaml already existed:

Object openstack/horizontest-clouds-config is already owned
by another HorizonTest controller horizontest-tests

This patch does two things:

  • Ensures that we do not create the modified clouds.yaml when it already exists.

  • Removes the duplicate implementation of of the Ensure*CloudsYAML function and replaces it with EnsureCloudsConfigMapExists.

Copy link

openshift-ci bot commented Nov 5, 2024

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

@openshift-ci openshift-ci bot added the approved label Nov 5, 2024
@lpiwowar lpiwowar changed the title [Horizon][Tobiko] Do not fail when clouds CM exists [OSPRH-11235] Do not fail when clouds CM exists Nov 5, 2024
@lpiwowar lpiwowar marked this pull request as ready for review November 11, 2024 09:44
@openshift-ci openshift-ci bot requested review from abays and viroel November 11, 2024 09:45
@lpiwowar
Copy link
Collaborator Author

/test precommit-check

Copy link
Collaborator Author

@lpiwowar lpiwowar left a comment

Choose a reason for hiding this comment

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

Thank you @kstrenkova! 🙏

controllers/common.go Show resolved Hide resolved
The Ensure[Horizon|Tobiko]CloudsYAML function was failing when
the CM containing the modified clouds.yaml already existed:

  Object openstack/horizontest-clouds-config is already owned
  by another HorizonTest controller horizontest-tests

This patch does two things:

  - Ensures that we do not create the modified clouds.yaml when it
    already exists.

  - Removes the duplicate implementation of of the Ensure*CloudsYAML
    function and replaces it with EnsureCloudsConfigMapExists.
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/71c97e8ffb6b455bb3c9a2ba24d20a9f

✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 07m 03s
podified-multinode-edpm-deployment-crc-test-operator FAILURE in 2h 52m 02s

@lpiwowar
Copy link
Collaborator Author

recheck

Copy link
Contributor

@kstrenkova kstrenkova left a comment

Choose a reason for hiding this comment

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

I have tested this change and it seems to be working. Lgtm.

Copy link

openshift-ci bot commented Nov 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kstrenkova, lpiwowar

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

@kstrenkova
Copy link
Contributor

/lgtm

Copy link

@evallesp evallesp left a comment

Choose a reason for hiding this comment

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

Some questions but in general lgtm.

helper *helper.Helper,
labels map[string]string,
) (ctrl.Result, error) {
const openstackConfigMapName = "openstack-config"

Choose a reason for hiding this comment

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

(blocking) question: I guess this one is not required right? Related as well with @kstrenkova
comment.
Found TestOperatorCloudsConfigMapName in the volumes.go files.

labels map[string]string,
) (ctrl.Result, error) {
const openstackConfigMapName = "openstack-config"
const testOperatorCloudsConfigMapName = "test-operator-clouds-config"

Choose a reason for hiding this comment

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

(non-blocking) question: would take the name from pkg.util.common.go ? So we don't need to maintain same const in two different places.

@openshift-merge-bot openshift-merge-bot bot merged commit 17b87f3 into openstack-k8s-operators:main Nov 18, 2024
8 checks passed
@lpiwowar
Copy link
Collaborator Author

/cherry-pick 18.0-fr1

@openshift-cherrypick-robot

@lpiwowar: #240 failed to apply on top of branch "18.0-fr1":

Applying: Do not fail when clouds CM exists
Using index info to reconstruct a base tree...
M	controllers/common.go
M	controllers/horizontest_controller.go
M	controllers/tobiko_controller.go
M	pkg/util/common.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/util/common.go
Auto-merging controllers/tobiko_controller.go
Auto-merging controllers/horizontest_controller.go
Auto-merging controllers/common.go
CONFLICT (content): Merge conflict in controllers/common.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Do not fail when clouds CM exists

In response to this:

/cherry-pick 18.0-fr1

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants