Skip to content

Proxito: remove old implementation #10660

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

Merged
merged 15 commits into from
Aug 28, 2023
Merged

Proxito: remove old implementation #10660

merged 15 commits into from
Aug 28, 2023

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Aug 23, 2023

Mostly removing old code, there is only one small custom addition

# Django doesn't include the leading slash in the path, so we normalize it here.
path = "/" + path
return self.serve_path(request, path)

Django doesn't capture the first slash, previously we were getting the path from request.path_info, that includes the slash! When a request is done to / the path is set to "" (empty), our code relies on that being /.

How to deploy this change

  • Deploy everything as usual, but don't run the projects 0105 migration!!!
  • After deploy is done, run the projects 0105 migration

@stsewd stsewd marked this pull request as ready for review August 23, 2023 20:50
@stsewd stsewd requested a review from a team as a code owner August 23, 2023 20:50
@stsewd stsewd requested a review from humitos August 23, 2023 20:50
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I took a quick look at this and it looks good. I didn't jump pretty deep into reviewing this code because it's mostly removal of an old implementation. I think we need to fix the tests and we are ready to go 🚀

Comment on lines +16 to +19
migrations.RemoveField(
model_name="project",
name="urlconf",
),
Copy link
Member

Choose a reason for hiding this comment

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

Won't deploy break if we remove this field? I assume the current production code is not using this field.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to run this migration after we have deployed the code.

Copy link
Member

Choose a reason for hiding this comment

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

OK! Make sure to make this pretty clear on the release project. We usually run migrations before deploying new code.

@stsewd stsewd merged commit 40bfe63 into main Aug 28, 2023
@stsewd stsewd deleted the remove-old-proxito-code branch August 28, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants