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: Remove unused files; add more tests; fix some Make targets and lint #332

Merged
merged 6 commits into from
May 16, 2023

Conversation

timmc-edx
Copy link
Contributor

@timmc-edx timmc-edx commented May 3, 2023

Remove unused files from root of each cookiecutter. These were not actually
used as they were not inside the {{cookiecutter.repo_name}} dir that
would lead to them being included.

More information on removals:

  • openedx.yaml removed from xblock cookiecutter root and
    Set appropriate tags in openedx.yaml #334 filed to cover
    actually adding that tag. (The rest of the file was either redundant
    with what's in python-template, or obsolete.)
  • requirements.txt was not used in CI or baking
  • Makefiles were not used. This is unfortunate because the IDA one actually
    had some useful tests in it. I've moved those to the ci.yml workflow for
    expediency but they should really be put into the unit tests at some
    point so that they can be run locally. A lot of the django-app Makefile
    seemed to have been written while the repo was still in development, and
    relied on packages that are not installed.

For the moved tests to pass, I had to make some fixes to the IDA template:

Other improvements:

  • Remove unused BAKE_OPTIONS from root Makefile
  • Use short version of BROWSER script in django-ida Makefile to
    match other Makefiles
  • Fix root Makefile so that it actually uses pip-sync rather than just
    installing it
  • Update JWT_PUBLIC_SIGNING_JWK_SET to be readable and copyable as JSON
    (as is likely to be done for edx-platform in the ongoing JWK work)
  • Made fake_translations Makefile target actually do something.

Merge checklist:
Check off if complete or not applicable:

  • Changelog record added
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, deadlines, tickets

@timmc-edx timmc-edx force-pushed the timmc/cleanup-makefile branch 2 times, most recently from 03610e3 to 297fac7 Compare May 4, 2023 22:25
@timmc-edx timmc-edx changed the title various fixes fix: Remove unused files; add more tests; fix IDA docs and lint May 4, 2023
@timmc-edx timmc-edx force-pushed the timmc/cleanup-makefile branch from 297fac7 to 1949d81 Compare May 4, 2023 22:28
@timmc-edx timmc-edx marked this pull request as ready for review May 4, 2023 22:46
Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

One question

Remove unused files from root of each cookiecutter. These were not actually
used as they were not inside the `{{cookiecutter.repo_name}}` dir that
would lead to them being included.

More information on removals:

- `openedx.yaml` removed from xblock cookiecutter root and
  #334 filed to cover
  actually adding that tag. (The rest of the file was either redundant
  with what's in python-template, or obsolete.)
- `requirements.txt` was not used in CI or baking
- Makefiles were not used. This is unfortunate because the IDA one actually
  had some useful tests in it. I've moved those to the ci.yml workflow for
  expediency but they should really be put into the unit tests at some
  point so that they can be run locally. A lot of the django-app Makefile
  seemed to have been written while the repo was still in development, and
  relied on packages that are not installed.

For the moved tests to pass, I had to make some fixes to the IDA template:

- Add missing `docs` target to Makefile
- Add some long-line lint suppression where needed

Other improvements:

- Remove unused `BAKE_OPTIONS` from root Makefile
- Use short version of `BROWSER` script in django-ida Makefile to
  match other Makefiles
- Fix root Makefile so that it actually uses pip-sync rather than just
  installing it
- Update `JWT_PUBLIC_SIGNING_JWK_SET` to be readable and copyable as JSON
  (as is likely to be done for edx-platform in the ongoing JWK work)
@timmc-edx timmc-edx force-pushed the timmc/cleanup-makefile branch from fe38a15 to d20ec7d Compare May 9, 2023 22:28
@timmc-edx
Copy link
Contributor Author

make docs is now failing. But guess what: The only reason it wasn't failing previously is that I hadn't added docs to the .PHONY list, so it was just saying make: 'docs' is up to date. and not actually building the docs. -.- Looking into it.

@timmc-edx
Copy link
Contributor Author

I think make docs is failing in the IDA output because the cookiecutter.requires_django variable isn't being respected. This means that the "Configure Django for autodoc usage" block isn't included, so importing urls.py fails.

Filed #339 for fixing
this later.
Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

Questions and suggestions for changelog

.github/workflows/ci.yml Show resolved Hide resolved
CHANGELOG.rst Outdated
Added
=====

- Improve testing for ``cookiecutter-django-ida`` (migrations, quality check, docker image build, docs build)
Copy link
Contributor

Choose a reason for hiding this comment

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

no docs build :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed :-(

Changed
=======

- Use short version of ``BROWSER`` script in django-ida Makefile to match others
Copy link
Contributor

Choose a reason for hiding this comment

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

Might also add "removed unused files in django-ida, django-app, and xblock"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added something like this

@timmc-edx
Copy link
Contributor Author

OK, got one last thing squared away. Ready for re-review.

Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

LGTM

@timmc-edx timmc-edx changed the title fix: Remove unused files; add more tests; fix IDA docs and lint fix: Remove unused files; add more tests; fix some Make targets and lint May 16, 2023
@timmc-edx timmc-edx merged commit 06f6914 into master May 16, 2023
@timmc-edx timmc-edx deleted the timmc/cleanup-makefile branch May 16, 2023 16:01
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.

2 participants