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 commit history with upstream #12

Merged
merged 293 commits into from
Dec 2, 2024

Conversation

DavidTheProgrammer
Copy link

@DavidTheProgrammer DavidTheProgrammer commented Dec 2, 2024

Description

This commit is necessary only to sync the git histories of the two projects. The initial update appears to have used a rebase thus creating new commits via playback and not actually merging the branches. The initial fix from this branch did however carry over all the file changes and hence the tests from the first time around are still valid. The only file change is the README.md file with a removal of the collectstatic command.

Related Issue

#4

How to test it locally

Check #5

Checklist

  • 🚀 is the code ready to be merged and go live?
  • 🛠 does it work (build) locally

Pull Request

  • 📰 good title
  • 📝good description
  • 🔖 issue linked
  • 📖 changelog filled out

Commits

  • commits are clean
  • commit messages are clean

Code Quality

  • 🚧 no commented out code
  • 🖨 no unnecessary logging
  • 🎱 no magic numbers
  • black was run locally (as part of the pre-commit hook)

Testing

  • ✅ added (appropriate) unit tests
  • 💢 edge cases in tests were considered
  • ✅ ran tests locally & are passing

jbothma and others added 30 commits February 24, 2022 12:58
Add variables to enable task error notification emails
…able-option

ordering variables while creating profile indicator
Tests for All details api enpoint and versions
Added validations to make sure user can only select version from valid list according to profile selected
Bumps [pillow](https://github.com/python-pillow/Pillow) from 7.1.0 to 9.0.1.
- [Release notes](https://github.com/python-pillow/Pillow/releases)
- [Changelog](https://github.com/python-pillow/Pillow/blob/main/CHANGES.rst)
- [Commits](python-pillow/Pillow@7.1.0...9.0.1)

---
updated-dependencies:
- dependency-name: pillow
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Updated dataset admin form to fix various breaking conditions
Bumps [django-tinymce](https://github.com/jazzband/django-tinymce) from 3.3.0 to 3.4.0.
- [Release notes](https://github.com/jazzband/django-tinymce/releases)
- [Changelog](https://github.com/jazzband/django-tinymce/blob/master/CHANGELOG.rst)
- [Commits](jazzband/django-tinymce@3.3.0...3.4.0)

---
updated-dependencies:
- dependency-name: django-tinymce
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
emre2038 and others added 21 commits October 18, 2023 14:46
…-2.2.3

Bump werkzeug from 2.0.2 to 2.2.3
Fixed bug in updating contentes via admin
…oints-download

Feature/cache profile points download
…imeout

Added GUNICORN_TIMEOUT env variable to allow for configurable timeouts
Add retry and timeout config to Q_CLUSTER to prevent parellel jobs
@DavidTheProgrammer DavidTheProgrammer requested a review from a team December 2, 2024 13:44
Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

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

LGTM (but I don't have all the context).

Comment on lines -53 to -54
docker-compose run --rm web python manage.py collectstatic --no-input

Copy link
Member

Choose a reason for hiding this comment

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

Does it mean this is run "somewhere" else or why don't we need it documented?

Copy link
Author

Choose a reason for hiding this comment

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

This is part of the local run configuration that I'll be re-documenting. As for now, we don't need to actually run this command as we don't have static resources really, so it works fine without it.

@DavidTheProgrammer
Copy link
Author

LGTM (but I don't have all the context).

Essentially we're just updating the commit history so that we have all commits from upstream. The change in the README.md, though valid, is just there because I wasn't sure if Github could create a PR with no file changes.

@DavidTheProgrammer DavidTheProgrammer merged commit df67be2 into staging Dec 2, 2024
3 of 5 checks passed
@kilemensi
Copy link
Member

Essentially we're just updating the commit history so that we have all commits from upstream. The change in the README.md, though valid, is just there because I wasn't sure if Github could create a PR with no file changes.

Cool @DavidTheProgrammer , are we planning on re-adding it in another PR or does the app run fine without the collectstatic command (in docker compose, I know Dokku has app.json to configure it).

@kilemensi
Copy link
Member

kilemensi commented Dec 2, 2024

... saw the "This is part of the local run configuration that I'll be re-documenting." comment so I'm assuming it's a separate PR.

@DavidTheProgrammer
Copy link
Author

are we planning on re-adding it in another PR or does the app run fine without the collectstatic command

The app runs fine without the collectstatic command at present.

... saw the "This is part of the local run configuration that I'll be re-documenting." comment so I'm assuming it's a separate PR.

No, it won't be re-added to the document in the other PR. But I will improve the README docs in that separate PR to reflect the current state of running it locally.

@kilemensi
Copy link
Member

The app runs fine without the collectstatic command at present.

Unlikely @DavidTheProgrammer , otherwise, there wouldn't have been a need of this pre-deploy script (that Dokku calls here).

What's the equivalent in docker compose world?

@DavidTheProgrammer
Copy link
Author

The app runs fine without the collectstatic command at present.

Unlikely @DavidTheProgrammer , otherwise, there wouldn't have been a need of this pre-deploy script (that Dokku calls here).

What's the equivalent in docker compose world?

Maybe my memory is failing me but I don't think I ever ran a collectstatic command and the app worked fine. I'll have to doublecheck these assumptions when rewriting the docs. In compose we can achieve something similar by providing multiple commands to the service:

...
    command: >
      bash -c "
      python manage.py collectstatic --noinput 
      && python manage.py runserver_plus 0.0.0.0:8000 --keep-meta-shutdown
      "

This works as expected. Other options include a custom startup script to execute as the command.

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