#30360 -- Support rotation of secret keys

@carltongibson Hi, I have forked the Django project, created a branch to work on this issue and I’m ready to start. In the email list you mentioned that I need to check out the PR & then rebase it, can you give me pointers on how to go about doing that?

Thanks!

Hey @carltongibson Seems like Vuyisile is already working on this one. If you need help with any other ticket, let me know :slight_smile:

@carltongibson Hi, I have forked the Django project, created a branch to work on this issue and I’m ready to start. In the email list you mentioned that I need to check out the PR & then rebase it, can you give me pointers on how to go about doing that?

Hey @terrameijar !

Welcome! I’ll suggest that you start reading the about the rebase command from here: Git - git-rebase Documentation

There will be an existing patch and when you get the latest changes from the master branch, you need to rebase those changes to the existing master.

Okay, I figured out how to check out the PR into my local branch. Please let me know how to proceed @carltongibson

Thanks @CuriousLearner , let me have a look!

Hi both. :wave:

Yes, so there’s the PR here. It’s old. There are some conflicts that need resolving.

I’d read a bit about rebasing https://git-scm.com/book/en/v2/Git-Branching-Rebasing — that’s probably the same link @CuriousLearner gave :smile:

The idea is that you replay the commit on top of the new base, hence rebase.

The goal is a single commit with the changes from the patch but updated for the latest state of the development branch.

Make sense?

The problem is there are 13 commits. To rebase I would first squash that to one commit. (Each commit that gets written onto the base commit has to resolve conflicts, so you can end up resolving the same conflict once for each commit, which is a pain. Better just to do it once.)

Then I’d try to rebase and resolve the conflict per-file. In theory it’s just git pull --rebase origin master but I don’t know how confident you are with git on the command line? Generally it’s much easier with a GUI. I use Git Fork https://git-fork.com/ — it’s very good, and you can optionally pay for it as and when you decide it’s earned your money (and you realise you’d be very grateful if they kept up developing it.)

With that the pull command has a rebase option which will step you into a graphical rebase.

Try all that. If you get stuck shout. :grinning: (!!!)

Once you’ve got the patch rebased, open a fresh PR, we’ll close the old one, and we can continue.

Sound OK?

@CuriousLearner I started a board of tickets here: https://github.com/orgs/django/projects/2 — have a look. (The runserver one would be cool… :wink: )

@terrameijar — worst case scenario — if the rebase is not feasible — is to copy the changes file by file manually. There ≈200 lines over 12 files, so it’s not too big.

Try to rebase: if you can’t resolve conflicts, revert just those files to the ones in master and then apply the patches by hand for them. (Likely most will just work…—but I didn’t try yet…)

Hey @carltongibson

Where do I start with that. Can you provide me some pointers on which existing files to look at and how to proceed?

Thanks!

Hi @CuriousLearner.

wsgiref has a simple server implementation https://docs.python.org/3/library/wsgiref.html#module-wsgiref.simple_server

We build on that in Django for the WSGIServer runserver uses.

If we can add a similar simple server to asgiref, we can add support to asgiref then we can add ASGI (async) support to runserver (without needing to rely on e.g. twisted, for Daphne, or uvicorn, or…)

We should probably start a new thread for that, since it’s a bigger project. But have a browse of the wsgiref source and let me know what you think.

Hi @terrameijar — how are you getting on? Did you a chance to sit down with this yet?

Once you’ve got the PR rebased and a fresh one open, we can work out what TODOs are to finish the feature. If you need help to get that far, do shout.

Kind Regards,

Carlton

@carltongibson Thanks. I had trouble figuring out rebase, I have never used rebase before, I managed to mess up my local environment a few times :slight_smile: Here’s where I am now, I’ve managed to squash the commits from the PR into one I think. Now I have conflicts. How do I proceed from here?

Hi @terrameijar.

Super! Rebase is one of those things to learn — doing it a few times is the only way.

Now I have conflicts. How do I proceed from here?

Did you download Git Fork? It’s got a visual merge tool built-in, which is handy.

So the idea is to apply the changes from the patch to (a new branch from) master. Trouble is there are changes since the patch was first made so it doesn’t apply cleanly. You then have to work out what the patch should now look like.

So for each conflicted file…

  • “Ours” should be the base branch. “Theirs” should be the patch — the one we’re applying on top. I always get confused about which is which.
  • Sometime just selecting the new version is correct. Maybe it needs moving up or down or rephrasing.
  • Other times you need to select the base version and then manually re-apply the changes.

You go through file-by-file, conflict-by-conflict until they’re all resolved.

Then you can commit :dancer:

And you should have a branch, a single commit ahead of master, which you can push to your fork on GitHub and open a PR. (Can help with that.)

How many conflicts are there? I’d try resolving a single file. Then another and so on. When doing this I’m always consulting the original diff on the PR Fixed #30360 -- Support rotation of secret keys by pelme · Pull Request #11198 · django/django · GitHub to make sure I adjust the patch correctly.

Make sense? Questions?

Have a go. Let me know if you have difficulties.

Kind Regards,

Carlton

I did everything in the commandline and to be fair, I still don’t understand rebase much. I’ll give it time, like you said, doing it a few times is the way.
Please have a look at the PR I submitted here, I squashed commits and accepted all incoming changes @carltongibson : https://github.com/django/django/pull/13618

Let me know what needs to be fixed and or improved. Thanks!

Hey @terrameijar. I just saw the PR. Well done. Let me have a proper look at it tomorrow, and I’ll comment there.

I did everything in the commandline …

You’re brave. :smile:

@carltongibson, How do I get the changes from your latest rebase on this issue? Is there anything I should do in git before running flake django tests?

Hi @terrameijar.

So I pushed to your branch on your fork. Let’s call those <branch> and <fork>.

You need to fetch those changes, and then (probably best way) reset so your HEAD (local copy of <branch>) points to the latest commit.

So… something like…

$ cd your/working/copy/of/django
$ git fetch <fork>  # This is the remote name, without `<`s
$ git switch <branch> # Checkout your local branch
$ git reset --hard <fork> <branch>

That last says Reset HEAD, throwing away any differences, to THIS point.

Worth having a read of git help reset — I use it all the time with and without the --soft, --mixed or --hard options.

Then I’d fix the flake8 errors and look at the test output to see what’s broken. Happy to dive in with you there.

Let me know how you get on!
C.

Thanks @carltongibson, I had no trouble doing the first three steps, but the last step gives me an error:

Cannot do hard reset with paths.

So I did a git reset --hard without the other options, ran tests (all OK, flake8 checks report nothing bad).

When I do git status I get:

Your branch is behind 'origin/fix_30360_pr' by 21 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)

Do I do a git pull here and push again?

Hi @terrameijar.

Do I do a git pull here and push again?

Yes, pull to the latest changes. The trick here is that we always want to rebase rather than create merge commits, which are a pain to get rid of — the can be fast-forwarded bit says that git pull will be enough. (Otherwise we git pull --rebase and resolve any conflicts.)

Once you’re updated, we’re fixing the flake8 errors and running the unit tests to resolve the errors.

@carltongibson thanks, I’ve fixed the flake8 errors that were showing up. I see lots of tests need fixing. Do you have any pointers on where I can start? Thanks

Hi @terrameijar. Good stuff. I saw you push a commit. :+1:

Basic strategy is pick one and see why it’s failing. Repeat until all pass. — So run the tests, pick one of the failures and dig in to understand it.

I need to run the tests and have a look myself, so give me a few days.
I’ll be able to comment more helpfully then. :smile: