-
Notifications
You must be signed in to change notification settings - Fork 20
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
base: main
Are you sure you want to change the base?
add nginx configurations for the mco-console #170
Conversation
@@ -65,6 +65,13 @@ spec: | |||
manageS3: | |||
default: false | |||
type: boolean | |||
overlappingCIDR: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this missed, earlier ?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed...
console/console.go
Outdated
return nil | ||
}); err != nil { | ||
}); err != nil && !errors.IsAlreadyExists(err) { | ||
// ConsolePlugin is a cluster-wide resource, and is not cleaned-up on operator un-install |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed...
There was a problem hiding this 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?
as discussed, leaving it as it is for now (that is MCO will be bringing both console pod and this ConfigMap)... |
There was a problem hiding this 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
console/console.go
Outdated
@@ -42,6 +45,18 @@ var ( | |||
serviceLabelKey = "app.kubernetes.io/name" | |||
) | |||
|
|||
func getNginxConfConfigMap(namespace string) apiv1.ConfigMap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func getNginxConfConfigMap(namespace string) apiv1.ConfigMap { | |
func getNginxConfConfigMap(namespace string) corev1.ConfigMap { |
package+version name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack...
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 <sanjaldhir@gmail.com>
c0f1120
to
18c22eb
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SanjalKatiyar 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 |
18c22eb
to
5e39faa
Compare
mco-console uses nginx for serving the UI assests. Add its configuration from the operator instead of build time. Signed-off-by: SanjalKatiyar <sanjaldhir@gmail.com>
5e39faa
to
8002d0d
Compare
generated output of "make bundle" command. Signed-off-by: SanjalKatiyar <sanjaldhir@gmail.com>
@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. |
okay, I checked... we already have... |
/hold |
Check: red-hat-storage/odf-operator#305 for more information.