OCM-2643: Allow default ingress configurations #242

tsorya · 2023-08-16T14:03:14Z

Tested with 4.14.0-ec.2 in candidate channel

provider/cluster_rosa_classic_resource.go

provider/cluster_rosa_classic_state.go

tsorya · 2023-08-28T18:17:10Z

/retest

openshift-ci-robot · 2023-09-03T08:57:52Z

@tsorya: This pull request references OCM-2643 which is a valid jira issue.

In response to this:

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/test-infra repository.

openshift-ci-robot · 2023-09-04T13:17:27Z

@tsorya: This pull request references OCM-2643 which is a valid jira issue.

In response to this:

Tested with 4.14.0-ec.2 in candidate channel

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/test-infra repository.

nirarg

Great work, just few comments
@sagidayan can you please also take a look?

provider/cluster_rosa_classic_resource.go

bardielle · 2023-09-11T11:45:32Z

/hold
would be merged after framework changes

nirarg · 2023-10-25T10:30:20Z

provider/defaultingress/ingress.go

+	"github.com/terraform-redhat/terraform-provider-rhcs/provider/common/attrvalidators"
+)
+
+const lowestDefaultIngressVer = "4.14.0-0"


I think its shouldn't be hard coded here, at least move it to be defined in the sdk library
If you agree, I'm ok with just opening ticket for changing it and merging without this

Not sure what you mean by "at least move it to be defined in the sdk library" ?

Is this value can be changed in the future?
If yes, it need to be managed from one place, for both cli and tf provider, no?

Can you point me to the right place? In anycase lets do it with another PR but need link to the place as i have no idea about the repo

This is managed by the api, if the user supplies an older version he'll face an error. So it should be ok to not have this in TF. It's there in the CLI mostly for the interactive mode and alerting the user, for TF he'll be alerted through the api error

@gdbranco When I ran it with an unsupported OCP version I got an error but it wasn't clear that this was the problem.... :

rhcs_default_ingress.default_ingress: Creating... ╷ │ Error: Failed building cluster default ingress │ │ with rhcs_default_ingress.default_ingress, │ on main.tf line 153, in resource "rhcs_default_ingress" "default_ingress": │ 153: resource "rhcs_default_ingress" "default_ingress" { │ │ Failed building autoscaler for cluster '27t5d6t120t5obdmf5948tc5nn6p40m3': status is 400, identifier is '400', code is 'CLUSTERS-MGMT-400' and operation identifier is │ 'e7959ab9-0ea5-42a1-a768-d1b9dd9d6daa': Can't update load balancer type on legacy supported ingress 'j2b3' in cluster '27t5d6t120t5obdmf5948tc5nn6p40m3'. For more │ information on how to be supported please check: https://access.redhat.com/node/7028653 ╵

Can we agree this is OCM issue or should we add cluster version param?

This is the expected error on OCM side though. It links back to a KCS on how to get supported if the user is trying to add changes he can't.

Maybe we could add to TF docs which are the attributes that can be modified in older versions. If the user tries to use those that are not available he'll face that error

provider/defaultingress/ingress.go

provider/clusterrosaclassic/validators.go

nirarg · 2023-12-04T11:35:27Z

provider/defaultingress/ingress.go

+	}
+
+	if !reflect.DeepEqual(state, plan) {
+		err := validateDefaultIngress(ctx, plan)


Is there any reason why not adding this validation in the plan stage?

not sure i understand, i assume it can be optimization but currently not sure what to do

All validations that can be done in the plan mode (before calling the backend) need to be done than
I don't think this critical for now, lets open ticket to change it later

provider/defaultingress/ingress.go

provider/defaultingress/notemptymap_validator.go

gdbranco · 2023-12-04T14:24:50Z

provider/defaultingress/ingress.go

@@ -0,0 +1,453 @@
+package defaultingress


Would you mind if we call it default application ingress?

Today we don't expose, but there's also an API ingress. So if we ever end up opening that it will be more complicated to adjust the terms 🤔

Or maybe we'll end up exposing through some attribute. Not sure 🤔 . Maybe we should just call it ingress then

I agree that ingress might be better package name here
The defaultingress might confuse
@tsorya any thoughts here?

As we are working only with default one I think we should stick with default in name. When we will have an option to work with not only default one we can change the package and add default to variables that can be set

Ok, I'm fine with pushing like this
Anyway, the package name is internal detail and can be changed without any issue

tsorya · 2023-12-04T15:58:43Z

/retest

nirarg · 2023-12-05T18:53:29Z

@tsorya thank you for the great work here
/approve
/lgtm

openshift-ci · 2023-12-05T18:53:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nirarg

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:

~~OWNERS~~ [nirarg]

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

tsorya · 2023-12-05T21:05:17Z

/retest

nirarg · 2023-12-06T08:03:56Z

/lgtm

openshift-ci bot requested review from nirarg and osherdp August 16, 2023 14:03

tsorya force-pushed the igal/ingress branch 2 times, most recently from 27b97fd to f56cf70 Compare August 22, 2023 09:21

bardielle reviewed Aug 22, 2023

View reviewed changes

provider/cluster_rosa_classic_resource.go Outdated Show resolved Hide resolved

gdbranco reviewed Aug 22, 2023

View reviewed changes

provider/cluster_rosa_classic_resource.go Outdated Show resolved Hide resolved

gdbranco reviewed Aug 22, 2023

View reviewed changes

provider/cluster_rosa_classic_state.go Outdated Show resolved Hide resolved

tsorya force-pushed the igal/ingress branch 4 times, most recently from e027d9c to a6077e8 Compare August 28, 2023 17:20

tsorya force-pushed the igal/ingress branch 2 times, most recently from 8d2ce7d to eb605fe Compare September 3, 2023 08:57

tsorya changed the title ~~DRAFT: ingress~~ OCM-2643: Allow default ingress configurations Sep 3, 2023

openshift-ci-robot added the jira/valid-reference label Sep 3, 2023

tsorya force-pushed the igal/ingress branch 5 times, most recently from d41b685 to de3e67d Compare September 4, 2023 08:46

nirarg reviewed Sep 5, 2023

View reviewed changes

provider/cluster_rosa_classic_resource.go Outdated Show resolved Hide resolved

provider/cluster_rosa_classic_resource.go Outdated Show resolved Hide resolved

provider/cluster_rosa_classic_resource.go Outdated Show resolved Hide resolved

openshift-ci bot added the do-not-merge/hold label Sep 11, 2023

openshift-merge-robot added the needs-rebase label Sep 16, 2023

tsorya force-pushed the igal/ingress branch from de3e67d to 181ad9e Compare September 26, 2023 12:15

openshift-merge-robot removed the needs-rebase label Sep 26, 2023

tsorya force-pushed the igal/ingress branch from 181ad9e to 6cf3e31 Compare September 26, 2023 12:17

nirarg reviewed Oct 25, 2023

View reviewed changes

tsorya force-pushed the igal/ingress branch from de324e3 to 23bed10 Compare November 4, 2023 20:10

openshift-ci bot removed the lgtm label Nov 4, 2023

gdbranco reviewed Nov 13, 2023

View reviewed changes

provider/defaultingress/ingress.go Outdated Show resolved Hide resolved

gdbranco reviewed Nov 13, 2023

View reviewed changes

provider/defaultingress/ingress.go Outdated Show resolved Hide resolved

tsorya force-pushed the igal/ingress branch 2 times, most recently from 2a3e344 to 1a4030e Compare December 3, 2023 15:20

bardielle reviewed Dec 3, 2023

View reviewed changes

provider/clusterrosaclassic/validators.go Show resolved Hide resolved

tsorya force-pushed the igal/ingress branch 2 times, most recently from 6c55df0 to 4708605 Compare December 4, 2023 09:35

nirarg reviewed Dec 4, 2023

View reviewed changes

gdbranco reviewed Dec 4, 2023

View reviewed changes

tsorya force-pushed the igal/ingress branch from 670e40a to 98063c4 Compare December 4, 2023 15:01

openshift-ci bot assigned nirarg Dec 5, 2023

openshift-ci bot added the lgtm label Dec 5, 2023

openshift-ci bot added the approved label Dec 5, 2023

tsorya added 5 commits December 5, 2023 23:11

OCM-2643: Allow default ingress configurations

6ad4759

Adding make generate result

4834a50

Delete TF resource on default ingress delete

7395ca3

Delete unused ocp version var

b3cafd8

Review fixes

d1257db

tsorya force-pushed the igal/ingress branch from 98063c4 to d1257db Compare December 5, 2023 21:11

openshift-ci bot removed the lgtm label Dec 5, 2023

openshift-ci bot added the lgtm label Dec 6, 2023

openshift-merge-bot bot merged commit 8dbe6c7 into terraform-redhat:main Dec 6, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCM-2643: Allow default ingress configurations #242

OCM-2643: Allow default ingress configurations #242

tsorya commented Aug 16, 2023 •

edited

Loading

tsorya commented Aug 28, 2023

openshift-ci-robot commented Sep 3, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Sep 4, 2023 •

edited by openshift-ci bot

Loading

nirarg left a comment

bardielle commented Sep 11, 2023

nirarg Oct 25, 2023

tsorya Oct 26, 2023

nirarg Oct 29, 2023

tsorya Oct 29, 2023

gdbranco Nov 13, 2023

bardielle Dec 4, 2023

tsorya Dec 4, 2023

gdbranco Dec 4, 2023

gdbranco Dec 4, 2023

nirarg Dec 4, 2023

tsorya Dec 4, 2023 •

edited

Loading

nirarg Dec 5, 2023

gdbranco Dec 4, 2023

gdbranco Dec 4, 2023

nirarg Dec 5, 2023

tsorya Dec 5, 2023

nirarg Dec 5, 2023

tsorya commented Dec 4, 2023

nirarg commented Dec 5, 2023

openshift-ci bot commented Dec 5, 2023

tsorya commented Dec 5, 2023

nirarg commented Dec 6, 2023

OCM-2643: Allow default ingress configurations #242

OCM-2643: Allow default ingress configurations #242

Conversation

tsorya commented Aug 16, 2023 • edited Loading

tsorya commented Aug 28, 2023

openshift-ci-robot commented Sep 3, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented Sep 4, 2023 • edited by openshift-ci bot Loading

nirarg left a comment

Choose a reason for hiding this comment

bardielle commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsorya Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsorya commented Dec 4, 2023

nirarg commented Dec 5, 2023

openshift-ci bot commented Dec 5, 2023

tsorya commented Dec 5, 2023

nirarg commented Dec 6, 2023

tsorya commented Aug 16, 2023 •

edited

Loading

openshift-ci-robot commented Sep 3, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Sep 4, 2023 •

edited by openshift-ci bot

Loading

tsorya Dec 4, 2023 •

edited

Loading