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

Fix app creation in source.wsgi, add regression test #6742

Merged
merged 3 commits into from
Feb 8, 2023

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Feb 6, 2023

Status

Ready for review

Description of Changes

  • Fix app creation in source.wsgi, add regression test
  • Use correct permissions when installing source.wsgi.logging in staging
  • Add basic testinfra smoke tests to verify SI and JI are up

See individual commit messages for further detail.

Fixes #6741.

Testing

How should the reviewer test this PR?

  • CI passes, including staging build
  • Revert the changes to source.py, run the newly added test_wsgi, verify it fails (i.e. it catches the regression)
  • Run make build-debs, install on a staging/prod environment, verify source interface works. Then run testinfra checks and verify they pass.

Deployment

Any special considerations for deployment? No, this fixes regressions that were never released.

Checklist

  • Linting (make lint) and tests (make test) pass in the development container
  • I have written a test plan and validated it for this PR

@zenmonkeykstop zenmonkeykstop self-assigned this Feb 6, 2023
@legoktm
Copy link
Member Author

legoktm commented Feb 6, 2023

Verified the source.py fix works in prod (and discovered #6743 along the way). Now figuring out why test_wsgi is passing locally and failing in CI and if we can also do a testinfra check too.

In journalist.wsgi and source.wsgi, we import the relvant `app` variable
so it can be executed by mod_wsgi. In bea3a0a this was accidentally
refactored to be under the `if __name__ == "__main__"` block, so it
wasn't available to be imported.

To prevent this from recurring, add a documentation comment making it
clear the variable needs to be globally accessible.

A regression test mostly verifies that the wsgi files can be executed
without errors. This required a slight modification to
`source_app.create_app()` which tries to scan a directory that doesn't
exist when tests are run. If the folder doesn't exist, we can skip that
entire routine since there's no data to obscure.

Fixes #6741.
@legoktm
Copy link
Member Author

legoktm commented Feb 7, 2023

staging failure is because hitting the smoke test against SI is giving a HTTP 500 Internal Server Error. :(

The file needs to be readable by www-data, so the "world" spot should be
set to 4 instead of 0.
Use curl on the host itself to verify that both the JI and SI
reply with HTTP 200 OK and have "Powered by..." in the HTML.

Notably this would've caught #6741.
@legoktm legoktm marked this pull request as ready for review February 7, 2023 21:59
@legoktm legoktm requested a review from a team as a code owner February 7, 2023 21:59
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

Changes look good, SI available in Qubes staging, and testinfra is green locally. LGTM

@zenmonkeykstop zenmonkeykstop merged commit d5f4417 into develop Feb 8, 2023
@zenmonkeykstop zenmonkeykstop deleted the 6741-wsgi branch February 8, 2023 15:54
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.

SI issuing a 500 error and then timing out on develop
2 participants