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

Changes to nginx @maintenance handler #168

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

Conversation

stick
Copy link
Member

@stick stick commented Apr 30, 2020

previous configuration would not display maintenance page if omero.web
wasn't running. This was likely due to the request being passed to the
@proxy_to_app named location and the configuration not being inherited
into that stanza.

This commit moves the error_page directive up to the server container
where possible or duplicates the directive into the named location
directive otherwise.

Additionally the return code is set to 502 to avoid situations where
returning the maintenance page causes a 200 return code and confusing
monitoring systems. (502 is always 502 with the current configuration.

@sbesson
Copy link
Member

sbesson commented May 4, 2020

Thanks for opening this @stick. The remaining failing test is the following which makes assumptions about nginx.conf vs nginx-location.conf

def testNginxLocationComment(self):
"""
Check the example comment in nginx-location matches the recommended
nginx configuration
"""
def clean(refname):
fn = path(__file__).dirname() / 'reference_templates' / refname
out = []
with open(fn) as f:
for line in f:
if re.match(r'##\s*\w', line):
out.append(line[2:].strip())
elif re.match(r'[^#]\s*\w', line):
out.append(line.strip())
return out
diffs = list(unified_diff(
clean('nginx.conf'), clean('nginx-location.conf'), n=0))
assert diffs == [
'--- \n',
'+++ \n',
'@@ -1,2 +0,0 @@\n',
'-upstream omeroweb {',
'-server 127.0.0.1:4080 fail_timeout=0;',
'@@ -7,0 +6 @@\n',
'+include /opt/omero/web/omero-web-location.include;',
'@@ -19 +18 @@\n',
'-proxy_pass http://omeroweb;',
'+proxy_pass http://127.0.0.1:4080;'
]

Given the divergence introduced in this PR between these two configuration files, is it worth trying to update the test or should this PR drop it so that we can review it @manics ?

@manics
Copy link
Member

manics commented May 7, 2020

I'm happy to see us remove nginx-location completely in the next breaking release.
omero.web.nginx_server_extra_config provides a lot of flexibility.

test/unit/test_web.py Outdated Show resolved Hide resolved
@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/omero-web-maintenance-page/38552/2

@joshmoore joshmoore mentioned this pull request Jun 3, 2020
@joshmoore
Copy link
Member

Re-opening with #174 in.

@joshmoore joshmoore closed this Jun 8, 2020
@joshmoore joshmoore reopened this Jun 8, 2020
@joshmoore
Copy link
Member

Still failing with:

E       AssertionError: assert ['--- \n', '+...+6 @@\n', ...] == ['--- \n', '+...+6 @@\n', ...]
2113E         At index 5 diff: '@@ -8 +6 @@\n' != '@@ -7,0 +6 @@\n'
2114E         Left contains 5 more items, first extra item: '@@ -20 +19 @@\n'
2115E         Use -v to get the full diff

Perhaps the changes from the uncommented run need adding?

cc: @stick @manics

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-python-superbuild-push#2. See the console output for more details.
Possible conflicts:

  • Upstream changes
    • test/unit/test_web.py

--conflicts

stick and others added 3 commits May 7, 2024 14:56
previous configuration would not display maintenance page if omero.web
wasn't running.  This was likely due to the request being passed to the
@proxy_to_app named location and the configuration not being inherited
into that stanza.

This commit moves the error_page directive up to the server container
where possible or duplicates the directive into the named location
directive otherwise.

Additionally the return code is set to 502 to avoid situations where
returning the maintenance page causes a 200 return code and confusing
monitoring systems.  (502 is always 502 with the current configuration.
Co-authored-by: Simon Li <orpheus+devel@gmail.com>
@knabar knabar force-pushed the nginx-maintenance-template branch from 219d9e7 to 06b0073 Compare May 7, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants