Should we have a test to determine if any git tracked files changed after bench build?

Frappe Framework
1

I’m running a few Frappe applications from the develop branch and I keep running into an issue often. When bench build is run, it modifies yarn.lock (and sometimes other files also), so the next time you run bench update, it fails.

Example from today as of commit 4c41ac1 in the helpdesk app.

[user@e601e9f88e7c helpdesk]$ git status
On branch develop
Your branch is up to date with 'upstream/develop'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   yarn.lock

no changes added to commit (use "git add" and/or "git commit -a")

This happens very frequently in the helpdesk and crm apps and I need to manually intervene, so I think there is a problem that needs to be solved.

It seems that when pull requests are submitted, contributors are forgetting to include yarn.lock, and then this gets accepted. I’m aware that there is a --reset flag, but this should only be used for worst-case scenarios.

Perhaps every frappe app should have a test in CI to ensure that bench build does not modify any files tracked by git, and if it did then the contributor should add the changed file for the test to pass. Anyone have any thoughts on this?

3 Likes
2

I’ve seen this before too quite a bit, and I never really understood the cause. I wonder how/why contributors would be forgetting yarn.lock. Wouldn’t it just be staged like anything else if changes happened? My understanding of CI practices is pretty limited.

3

When I do development, I stage each individual file or hunk manually. Before I push a commit, I do pay attention to what is remaining in my worktree. Seeing changed files after a build, originally I thought this was just a slip-up some people make.

It is a bit odd to see how often this is happening with yarn.lock specifically, so I think with different versions of NodeJS or yarn, there might be subtle differences in how the dependency tree is generated, and maybe some contributors intentionally leave that out thinking it should build fine without it, which is why I’m thinking this should be checked in CI.

Related GitHub Issues:

It appears like changing yarn install to yarn install --frozen-lockfile in the postInstall script in package.json for both crm and helpdesk should fix this issue, but it looks like the crm app is also reinstalling dependencies when a workspace is enabled.

2 Likes