-
Notifications
You must be signed in to change notification settings - Fork 11
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
Expose Minio Console and API via Ingress #42
base: main
Are you sure you want to change the base?
Conversation
There's a few linting errors (see |
if interfaces["ingress"]: | ||
interfaces["ingress"].send_data( | ||
{ | ||
"prefix": "/", |
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.
This'll define the endpoint that we claim on the ingress, as written we've claimed the root (or really, http://INGRESS_URL/
). It probably makes the most sense that this be configurable via a config option. The config option could accept anything that would be valid in a URL (a good default value could be /minio
).
src/charm.py
Outdated
) | ||
interfaces["ingress"].send_data( | ||
{ | ||
"prefix": "/", |
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.
Can this prefix
and the above one be the same? I don't know this for sure, but my expectation is that they will conflict. I think these might need to be something like "prefix": "/SOME_VALUE_FROM_CONFIG/console"
and "/SOME_VALUE_FROM_CONFIG/api"
(or whatever good name you suggest)
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 am not sure either. I didn't consider it because the ports are different, but it doesnt serve it on the same port I would assume
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 think the only external facing thing is the prefix
, and then the port
is used when routing to the kubernetes service (eg: INGRESS/PREFIX
routes to SERVICE:PORT
inside kubernetes)
@@ -309,3 +309,65 @@ def test_minio_console_port_args(harness): | |||
"--console-address", | |||
":9999", | |||
] | |||
|
|||
def test_install_with_all_inputs(harness): |
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.
This test can work as a general "can I successfully invoke pod_spec" test, but I think we are missing something to actually check whether this charm has published what we expect to the relation. For example, like here
@jardon it is missing the following entries:
Otherwise it will not work if the relation is added after minio has been deployed |
Adds an
ingress
relation to that sends the console and apifixes #40