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

Skip .gitignore in existing monorepo #129

Merged
merged 1 commit into from
Jul 6, 2023
Merged

Conversation

simonihmig
Copy link
Collaborator

The .gitignore is used for ignoring files that are moved to the addon from the root folder at build time. But this setup does not apply for an existing monorepo (all root files are ignored), so we don't need the .gitignore (and it does harm, as it matches the README.md file)

@simonihmig simonihmig added the bug Something isn't working label Jul 3, 2023
@simonihmig simonihmig requested a review from NullVoxPopuli July 3, 2023 20:29
if (this.isExistingMonorepo) {
// The .gitignore is used for ignoring files that are moved to the addon from the root folder at build time
// But this setup does not apply for an existing monorepo (all root files are ignored), so we don't need the .gitignore
files = files.filter((filename) => filename !== '__addonLocation__/gitignore');
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Jul 3, 2023

Choose a reason for hiding this comment

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

in an existing monorepo we do want to keep the addon's gitignore tho.

each package which can generated ignored files benefits from a local gitignore

Maybe I'm missing something?
is this supposed to be the top-level gitignore?

Copy link
Collaborator Author

@simonihmig simonihmig Jul 5, 2023

Choose a reason for hiding this comment

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

The situation changed a bit with merging #136. And actually I am not sure the changed to the addon's .gitignore here were what we really want. I probably missed the fact that this was the addon's local .gitignore, and not the root one...

Previously, the local .gitignore only had this:

/README.md
/LICENSE.md

This was necessary to ignore the files that we copy as part of the build here.

However, in an in-monorepo situation, this is different. We don't copy these files anymore as part of the build, but move them into the addon folder when the blueprint runs. This is then their intended location. But then we must not gitignore them anymore. And as stated above, when those two entries are the only ones in the .gitignore, then taht file does not make any sense anymore. That's why it is not created in this PR.

Back to the changes in #136: I think they should be reverted, that's what I'm doing at #141. Let's discuss that topic over there, in case you disagree...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given that #141 has been merged, the change here makes sense again I think. The addon's gitignore again contains only the Readme and License files, which in an existing monorepo must not be ignored as explained above, so we can delete the addon's .gitignore. The "Introduce workspace isolation" PR we talked about will need to adjust that eventually...

The .gitignore is used for ignoring files that are moved to the addon from the root folder at build time. But this setup does not apply for an existing monorepo (all root files are ignored), so we don't need the .gitignore (and it does harm, as it matches the README.md file)
@simonihmig simonihmig force-pushed the fix-gitignore-in-monorepo branch from a7c96aa to 2dedaba Compare July 6, 2023 14:29
@simonihmig simonihmig merged commit a57ed02 into main Jul 6, 2023
@simonihmig simonihmig deleted the fix-gitignore-in-monorepo branch July 6, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants