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

Accumulated build files since #2411 #2673

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pablobm
Copy link
Collaborator

@pablobm pablobm commented Oct 4, 2024

Not sure what to make of this one. I was getting these build files in a branch I was working on. Eventually I realised this was happening in main too. Seems to be the case since merging #2411, and I think changes might have been accumulating over time?

So... should these be committed then?

I can't add @littleforest as a reviewer for some reason, so pinging here instead.

@goosys
Copy link
Contributor

goosys commented Oct 4, 2024

I think it might be necessary to add the app/assets/builds directory to the .gitignore .

@nickcharlton
Copy link
Member

Hm, the intent is that we'd update builds once we cut a release. Which hasn't been a while.

How much of a problem is this creating when developing between releases?

@goosys
Copy link
Contributor

goosys commented Oct 8, 2024

By updating modules with Dependabot, running bin/dev on the latest main branch causes changes in the builds output, resulting in differences.
I have to manually delete these changes each time to avoid including them in commits, which is a bit inconvenient.

@nickcharlton
Copy link
Member

Ah yeah, that'd do it.

This feels like something we can automate in CI so that we catch it when working on the PR. I'll give that some thought.

nickcharlton added a commit that referenced this pull request Oct 8, 2024
In #2673, we discussed catching changes in the `builds` directory as
being quite annoying in development. This can happen when people
contribute changes (and should probably also commit their build changes
to make future development easier), plus also when dependencies change.

Catching this with `diff-check` should mean that we see this at the
point of introduction, rather than later on.
@nickcharlton
Copy link
Member

nickcharlton commented Oct 8, 2024

I opened #2680 to try something out for this, and it was easier than I thought. I wondered if I'd need to make some modifications to diff-check to do this, but it worked first time.

@pablobm
Copy link
Collaborator Author

pablobm commented Oct 19, 2024

Another issue with not checking in the assets is that gems included from Git (eg: gem "administrate", git: ...) do not get the correct CSS. For example, I just built a quick Administrate app pulling from main, and it doesn't include the changes introduced at #2558

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.

3 participants