diff --git a/README.md b/README.md index 2dfc5193f..789cd31c8 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ Samfundet4 is the latest and greatest iteration of samfundet.no. It's built usin ### Workflow -- [Work Methodology](./docs/work-methodology.md) – How to contribute to the project +- [Work Methodology and reviewing pull requests](./docs/work-methodology.md) – How to contribute to the project - [Useful Commands](./docs/useful-commands.md) - [Useful Docker aliases](./docs/docker-project-specific-commands.md) - [Common error messages](./docs/common-errors.md) diff --git a/docs/approving-dependabot-prs.md b/docs/approving-dependabot-prs.md index 94f6e0ede..ad8cc5aeb 100644 --- a/docs/approving-dependabot-prs.md +++ b/docs/approving-dependabot-prs.md @@ -9,6 +9,8 @@ When approving dependabot pull requests and merging them, the main goal is to NO - Generally, to SAVE TIME you should check and approve dependabot pull requests in the order of when the last one was merged. That is; approve and merge a dependabot pull request, then go on to the next one. +- You will save time by running the pipeline and merging up to date master into the dependabot branch (change branch) locally. That is; not doing it in GitHub. + - Tell Web when you approve and merge dependabot pull requests so that they can rebuild their docker instance after they pull from the new version of the master branch. - If there are major version changes (e.g., Django 5.2 --> 5.3, not Django 5.2.2 --> 5.2.4) one should take greater care when checking the pull request. @@ -19,17 +21,13 @@ When approving dependabot pull requests and merging them, the main goal is to NO - Package version upgrades are created by the package developer teams in such a way that they don't break projects using recent versions of the package, therefore upgrading often is a good strategy for avoiding that package upgrades come with a lot of work fixing problems in our code as a consequence of extensive changes. -## 1) Make sure that the dependabot branch is up to date with the base branch (master) - -You can do this by checking that the branch does not need to be updated through GitHub. GitHub shows you this close to where you find the button for merging pull requests. +## 1) Make sure that the change branch is up to date with the base branch (master) -> The pipeline requires that dependabot's branch is up to date with the base branch (master), but there is also a problem where it seems like dependabot does not have permission to deploy to Chromatic instances. This problem can be resolved by updating the branch because we have permissions to deploy Chromatic instances. +Checkout the change branch locally and merge the most current branch into local master. -This button updates the current branch (dependabot's branch) with the base branch: -![Update base branch](./assets/img-update-base-branch.png) - -*Updating the current branch with the base branch is the same as merging the newest version of master into the current branch, so this is also something you can do through Git, the GitHub CLI or some other Git client*. +*You can also do this by checking that the branch does not need to be updated through GitHub. GitHub shows you this close to where you find the button for merging pull requests* +> The GitHub pipeline requires that the change branch is up to date with the base branch (master), but there is also a problem where it seems like dependabot does not have permission to deploy to Chromatic instances. This problem can be resolved by updating the change branch (we have permissions to deploy Chromatic instances). ## 2) Make sure that the changes do not break Samfundet4 @@ -37,11 +35,17 @@ When approving dependabot pull requests you need to make sure that upgrading doe You can make sure that the changes do not break Samfundet4 by: -1) Checking that the dependabot pull request passes our GitHub pipeline, in most cases specifically backend and frontend tests +1) Checking that the dependabot pull request passes our GitHub pipeline +> You can run pipeline commands in Docker. See these commands [here](./docker-project-specific-commands.md). +> ```pipeline``` is most useful. You will have to run frontend commands one by one separately. + +2) Checkout the change branch localy -2) Checking that generally Samfundet4 runs as expected +3) Run `docker compose up --build` to build your local Samfundet4 instance with the updated dependencies. -3) Check Samfundet4 functionality which specifically uses the dependency that is being upgraded +4) Checking that generally Samfundet4 runs as expected + +5) Check Samfundet4 functionality which specifically uses the dependency that is being upgraded ###### How to check usage of specific dependencies: @@ -59,6 +63,8 @@ These changes will highlight what dependency is being upgraded and maybe if ther Search for the dependency name in Samfundet4 and check manually that the Samfundet4 functionality works as expected. +> ⚠️ In a lot of cases it is not possible to find direct usage of dependencies and packages in the high-level codebase of Samfundet4. In cases where you don't find direct usage you will have to get familiar with what the dependency/package does and manually test Samfundet4 functionality that you think probably rely on the dependency/package. + Most dependencies/packages have multiple different functions we are using, so if you want to be extra diligent you should check parts of Samfundet4 functionality that rely on different distinct functions of the dependency/package. *For example, in case tanstack is being upgraded and we use `useQuery` and `useMutation`*. When being this diligent you should check what parts of the dependency/package code are being affected by the upgrade (see tips). > Checking that it works as expected can in some cases be challenging because some parts of Samfundet4 are broken because of our code and not the dependency code. @@ -71,8 +77,6 @@ If you are unsure about whether the pull request should be merged (maybe because Remember; in development nothing bad can really happen. The worst thing that can happen is other developers are affected by a broken master branch. This is in most cases fairly quick to fix. You can't break important stuff like event sales by merging into a development branch. - - # Interpreting exactly what dependency/package is being upgraded: **The diff file in `frontend/yarn.lock` of a pull request created by dependabot might look something like this:** @@ -99,8 +103,6 @@ In the example above (from [this PR](https://github.com/Samfundet/Samfundet4/pul In other cases, let's say if `axios@npm` is being upgraded, we might be using the dependency directly. - - **A diff file in `frontend/package.json` of a pull request created by dependabot might look something like this:** In [this PR](https://github.com/Samfundet/Samfundet4/pull/1889/files) react-markdown is being upgraded. This is a dependency we are using directly [(here)](https://github.com/Samfundet/Samfundet4/blob/dd72442032fa460e35e30fcc1976854a0ed17e7f/frontend/src/Components/SamfMarkdown/SamfMarkdown.tsx). @@ -115,9 +117,12 @@ form-data: 4.0.2 → 4.0.3 (tiny jump) → only yarn.lock changes Direct dependency (depends on constraint): "react": "^18.0.0" and 18.2.1 → 18.3.0 → only yarn.lock changes (satisfies ^18.0.0) + "react": "^18.0.0" and 18.2.1 → 19.0.0 → both files change (doesn't satisfy ^18.0.0) + "react": "~18.2.0" and 18.2.1 → 18.3.0 → both files change (doesn't satisfy ~18.2.0) + **A diff file in `backend/poetry.lock` of a pull request created by dependabot might look something like this:** The package being upgraded in @@ -144,4 +149,4 @@ files = [ The package upgrade in [this PR](https://github.com/Samfundet/Samfundet4/pull/1910/files) is a somewhat large upgrade, where Django is being upgraded from 5.1.9 to 5.2.4. This affects the whole backend, but as long as we make sure to check, approve and merge (upgrade) package versions regularly there is a low chance for the backend to break. -Whether `pyproject.toml` gets updated depends on if the dependency is direct or indirect, and for direct dependencies, whether the new version satisfies the existing version constraint. +Whether `pyproject.toml` gets updated depends on if the dependency is direct or indirect, and for direct dependencies, whether the new version satisfies the existing version constraint. \ No newline at end of file diff --git a/docs/assets/img-update-base-branch.png b/docs/assets/img-update-base-branch.png deleted file mode 100644 index 2839d41ec..000000000 Binary files a/docs/assets/img-update-base-branch.png and /dev/null differ diff --git a/docs/common-errors.md b/docs/common-errors.md index c016372e6..00de1e0bc 100644 --- a/docs/common-errors.md +++ b/docs/common-errors.md @@ -8,6 +8,19 @@ If running the project in Docker, many issues are solved by running a `docker sy If you find something that is not listed, please add it to this document! +## Seeding problems + +If you are having problems with seeding the database there is usually one single reason; you current local database is out of date. You can solve it by deleting the current database, then seed again. + +``` bash +# in Samfundet4/backend/database +rm -f docker.db.sqlite3 +``` + +If you are still having problems then you might need to recreate the database by recraetingt the database with the Django migrate command. See how to do this [here](./docker-project-specific-commands.md). + +Still not seeding? Try `docker compose down`, then `docker compose build` and finally `docker compose up`. + ## Entrypoint.sh ### Error message: ``` diff --git a/docs/work-methodology.md b/docs/work-methodology.md index ba06d2a84..02990a871 100644 --- a/docs/work-methodology.md +++ b/docs/work-methodology.md @@ -29,15 +29,15 @@ 9. Owner of the PR merges it. -# Approving pull requests +# Reviewing pull requests -## Approving pull requests from dependabot +## Reviewing pull requests from dependabot - [Read about](./approving-dependabot-prs.md) how to go about approving dependabot pull requests. -## Approving Web pull requests +## Reviewing Web pull requests -Many of the principles for reviewing pull requests created by someone in Web are the same as those for [approving pull requests from Dependabot](./approving-dependabot-prs.md). The main goals are to avoid breaking functionality, help improve code quality, and validate that the code additions and changes are necessary and actually solve the intended issue. +Many of the principles for reviewing pull requests created by someone in Web are the same as those for [reviewing pull requests from Dependabot](./approving-dependabot-prs.md). The main goals are to avoid breaking functionality, help improve code quality, and validate that the code additions and changes are necessary and actually solve the intended issue. This is done by reading the code, pulling the branch and manually verifying the the changes or additions are doing what they are indended to do. In some cases you might want to make temporary changes to the code to check for potential imporvements. \ No newline at end of file