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

add nginx configurations for the mco-console #170

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SanjalKatiyar
Copy link
Contributor

Check: red-hat-storage/odf-operator#305 for more information.

@@ -65,6 +65,13 @@ spec:
manageS3:
default: false
type: boolean
overlappingCIDR:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why overlappingCIDR is part of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

is this missed, earlier ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think some other PR missed this... this is not due to this PR...

Copy link
Contributor

Choose a reason for hiding this comment

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

ack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed...

return nil
}); err != nil {
}); err != nil && !errors.IsAlreadyExists(err) {
// ConsolePlugin is a cluster-wide resource, and is not cleaned-up on operator un-install
Copy link
Contributor

Choose a reason for hiding this comment

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

it is CreateOrUpdate only, is this will raise an already exists error?

Copy link
Contributor Author

@SanjalKatiyar SanjalKatiyar Apr 25, 2023

Choose a reason for hiding this comment

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

I did not test this... but seems to me:
Let;s say I installed MCO for the first time >> operator will create ConsolePlugin CR for me.
Now, I deleted MCO operator >> ConsolePlugin CR did not get deleted as a part of operator or NameSpace deletion.
Now, I install MCO again >> operator will create ConsolePlugin again, but this time it will already be existing as it was not automatically cleaned up at time of operator deletion... it that case it will return "alreadyExists" error, and here I am ignoring that error.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok! but I did this multiple times, I never faced this issue, I think it was because of createOrUpdate, anyway you are gonna this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually it make sense... u r right "createOrUpdate" will handle this... "IsAlreadyExists" check is not needed...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed...

Copy link
Contributor

@umangachapagain umangachapagain left a comment

Choose a reason for hiding this comment

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

Instead of creating a configmap through operator, can we add it to the bundle?
That way it will always be present when console is started?

@SanjalKatiyar
Copy link
Contributor Author

Instead of creating a configmap through operator, can we add it to the bundle? That way it will always be present when console is started?

as discussed, leaving it as it is for now (that is MCO will be bringing both console pod and this ConfigMap)...

Copy link
Member

@vbnrh vbnrh left a comment

Choose a reason for hiding this comment

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

Please add the required RBACs

@@ -42,6 +45,18 @@ var (
serviceLabelKey = "app.kubernetes.io/name"
)

func getNginxConfConfigMap(namespace string) apiv1.ConfigMap {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func getNginxConfConfigMap(namespace string) apiv1.ConfigMap {
func getNginxConfConfigMap(namespace string) corev1.ConfigMap {

package+version name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to point out, "apiv1" was used at multiple places in this file...

overlappingCIDR:
default: false
description: OverlappingCIDR should be set to true if the peer clusters
have overlapping Pod or Service CIDR. This will enable storage clusters
Copy link
Member

Choose a reason for hiding this comment

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

Remove any updates to this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack...

Once mco-console Deployment is deleted, corresponding Service
should get deleted as well.

Signed-off-by: SanjalKatiyar <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 26, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SanjalKatiyar
Once this PR has been reviewed and has the lgtm label, please ask for approval from vbnrh. For more information see the Kubernetes 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

mco-console uses nginx for serving the UI assests.
Add its configuration from the operator instead of build time.

Signed-off-by: SanjalKatiyar <[email protected]>
generated output of "make bundle" command.

Signed-off-by: SanjalKatiyar <[email protected]>
@SanjalKatiyar
Copy link
Contributor Author

Please add the required RBACs

@vbnrh don't we do it for controller's reconcile only ? we don't have specific controller for console, it run once during init.

@vbnrh
Copy link
Member

vbnrh commented Apr 26, 2023

Please add the required RBACs

@vbnrh don't we do it for controller's reconcile only ? we don't have specific controller for console, it run once during init.

Not sure if it is added there.
If you've tested it and it's all working then the RBACs should be there.

@SanjalKatiyar
Copy link
Contributor Author

Please add the required RBACs

@vbnrh don't we do it for controller's reconcile only ? we don't have specific controller for console, it run once during init.

Not sure if it is added there. If you've tested it and it's all working then the RBACs should be there.

okay, I checked... we already have...

@SanjalKatiyar
Copy link
Contributor Author

/hold
needs testing...

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