Skip to content

Conversation

joschi36
Copy link

@joschi36 joschi36 commented Apr 24, 2025

What this PR does / why we need it:

Makes it possible to configure a HCP cluster without a CNI, like with rosa cluster create --no-cni

Implementing it like the rosa CLI does it: https://github.com/openshift/rosa/blob/4f425f6d857bb7b1ddb7af8fc69a25d25ce8fae3/pkg/ocm/clusters.go#L939

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story (OCM-xxxx):
Fixes #885

Change type

  • New feature
  • Bug fix
  • Build
  • CI
  • Documentation
  • Performance
  • Refactor
  • Style
  • Unit tests
  • Subsystem tests

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.

Copy link

openshift-ci bot commented Apr 24, 2025

Hi @joschi36. Thanks for your PR.

I'm waiting for a terraform-redhat member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@hunterkepley
Copy link
Member

/ok-to-test

@joschi36 joschi36 marked this pull request as ready for review May 16, 2025 16:00
@openshift-ci openshift-ci bot requested review from gdbranco and vkareh May 16, 2025 16:00
@hunterkepley
Copy link
Member

hunterkepley commented Jun 6, 2025

Hi @joschi36 apologies for getting to this so late, there's low manpower for ROSA CLI + TF team currently.

If you could amend your commit + change the MR title to follow the format of TICKET/ISSUE-#### | type : Implement no-cni for HCP clusters that would get the ball rolling on the automated tests

In this case, ISSUE-885 | feat: Implement no-cni for HCP clusters would work fine

@hunterkepley
Copy link
Member

Content of the MR looks good to me. If you could run make generate as well that would clear another hurdle in the testing. Then we can see what the tests say

@joschi36 joschi36 changed the title Implement no-cni for HCP clusters ISSUE-885 | feat: Implement no-cni for HCP clusters Jul 8, 2025
Copy link

openshift-ci bot commented Jul 8, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vkareh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@joschi36
Copy link
Author

@hunterkepley I've implemented your requests. Does this look alright now?

@hunterkepley
Copy link
Member

@joschi36 I believe last thing would be to rebase the MR with master- tests are failing because the go version is behind the testing environment

@joschi36
Copy link
Author

@hunterkepley I have rebased to upstream/main. Tests still seem to fail because of the go version. Did I miss something?

Copy link

openshift-ci bot commented Aug 12, 2025

@joschi36: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/unit 5f22dc0 link true /test unit
ci/prow/e2e-presubmits-rosa-sts-private-critical-high-presubmit 5f22dc0 link true /test e2e-presubmits-rosa-sts-private-critical-high-presubmit
ci/prow/e2e-presubmits-rosa-hcp-advanced-critical-high-presubmit 5f22dc0 link true /test e2e-presubmits-rosa-hcp-advanced-critical-high-presubmit
ci/prow/e2e-presubmits-rosa-sts-advanced-critical-high-presubmit 5f22dc0 link true /test e2e-presubmits-rosa-sts-advanced-critical-high-presubmit
ci/prow/e2e-presubmits-rosa-hcp-private-critical-high-presubmit 5f22dc0 link true /test e2e-presubmits-rosa-hcp-private-critical-high-presubmit

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.

@hunterkepley
Copy link
Member

/retest-required

@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

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.

@hunterkepley
Copy link
Member

hunterkepley commented Sep 26, 2025

@joschi36 Hey there, apologies for the late reply I was quite busy (im the only eng who is working on RHCS TF for the past year)

Seems like we need 1 more rebase (conflict with provider/clusterrosa/hcp/resource.go) - 2 of the test suites should indeed fail, because 2 of those test suites are broken entirely, This would be ci/prow/unit and ci/prow/e2e-presubmits-rosa-sts-advanced-critical-high-presubmit

Those 2 test suites should be failing but the rest should be passing (originally, you had 3 other test suites failing)

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.

Support for deploying ROSA HCP clusters without CNI
3 participants