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

Parameterization of the client_max_body_size directive in Nginx #2014

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

shellclear
Copy link

@shellclear shellclear commented Feb 7, 2025

SUMMARY

This commit aims to parameterize an Nginx configuration that is currently hardcoded to 5M, causing issues with file uploads.

ISSUE TYPE
  • New or Enhanced Feature
ADDITIONAL INFORMATION

With this change, it will be possible to set the value of the client_max_body_size directive during the creation of the AWX CR or modify an existing AWX instance running in the cluster.

The default value for the directive remains 5M.

apiVersion: awx.ansible.com/v1beta1
kind: AWX
metadata:
  name: example-awx1
  namespace: awx
spec:
  nginx_client_max_body_size: 10

@shellclear
Copy link
Author

#2013

@fosterseth
Copy link
Member

@rooftopcellist I believe this is configurable through the AAP installer, so maybe makes sense to allow it here too

@shellclear shellclear requested a review from fosterseth February 7, 2025 19:57
@rooftopcellist
Copy link
Member

I agree this should be added, and I fully expect the diff here to work. But I am torn because this same param is configurable in galaxy-operator, but there it is type string not an int that expects the "M" units on the end to be passed on the string.

I think what you have here in this PR is the better approach, but the inconsistency across ansible operators may be confusing.

We plan to create a new apiVersion for all of these operators to make a consistent parameter interface. When we do, we should modify the galaxy-operator to use this approach (int and adding on the units in the template).

@rooftopcellist
Copy link
Member

@fosterseth If you agree with this, feel free to merge next week.

@shellclear
Copy link
Author

I agree this should be added, and I fully expect the diff here to work. But I am torn because this same param is configurable in galaxy-operator, but there it is type string not an int that expects the "M" units on the end to be passed on the string.

I think what you have here in this PR is the better approach, but the inconsistency across ansible operators may be confusing.

We plan to create a new apiVersion for all of these operators to make a consistent parameter interface. When we do, we should modify the galaxy-operator to use this approach (int and adding on the units in the template).

for me, it needs to be an integer. I'll push a new PR to change the parameter in the galaxy-operator so that we can keep the operator aligned.

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.

3 participants