-
Notifications
You must be signed in to change notification settings - Fork 274
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
feat(core): make module scans more configurable and ignores more robust #1019
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, good job Jón!
Aside few questions it's good to go for me. One last question: did you think about what happens if somebody adds the same path in both include
and exclude
?
E.
garden-service/src/util/fs.ts
Outdated
|
||
const VALID_CONFIG_FILENAMES = ["garden.yml", "garden.yaml"] | ||
const metadataFilename = "metadata.json" | ||
export const defaultDotIgnoreFiles = [".gitignore", ".gardenignore"] | ||
export const fixedExcludes = [".git", ".garden", "debug-log-*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original ignore was debug-info-*
not debug-log-*
or you perhaps meant *.log
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yeah I'll fix that.
} | ||
|
||
return paths | ||
export async function findConfigPathsInPath( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I suggest a more explicit name like findModulePaths
or findGardenModulePaths
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also there are missing @param
s in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These find all config paths, not just modules, so I went for the more generic term.
Also, we generally don't use @param
annotations, feels unnecessary in most cases with TypeScript imo.
@@ -622,6 +629,12 @@ export class Garden { | |||
return version | |||
} | |||
|
|||
async scanForConfigs(path: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment to explain reasons and context and to not mistake this function with the one down?
I didn't consider the case where |
This adds `dotIgnoreFiles`, `modules.include` and `modules.exclude` fields to the project config. The first allows you to specify which "dot ignore" files to use across you project. To match current behavior, `dotIgnoreFiles` defaults to `.gitignore` and `.gardenignore`. The feature is also more robust now because we properly use git to evaluate the files in all cases now, including handling nested ignore files. This meant getting rid of the `Ignorer` package, which was always quite flimsy anyway, and instead using the VcsHandler when scanning the project, much like module sources. Secondly, we allow explicit filtering of which directories to scan for modules, using the `modules.include/exclude` keys. This allows us to control project scanning, without blanket ignoring files that we may actually want to be part of module builds.
1e1c893
to
cd9a3ab
Compare
Thanks Jón. |
This adds
dotIgnoreFiles
,modules.include
andmodules.exclude
fields to the project config.
The first allows you to specify which "dot ignore" files to use across
you project. To match current behavior,
dotIgnoreFiles
defaults to.gitignore
and.gardenignore
. The feature is also more robustnow because we properly use git to evaluate the files in all cases now,
including handling nested ignore files.
This meant getting rid of the
Ignorer
package, which was always quiteflimsy anyway, and instead using the VcsHandler when scanning the
project, much like module sources.
Secondly, we allow explicit filtering of which directories to scan for
modules, using the
modules.include/exclude
keys. This allows us tocontrol project scanning, without blanket ignoring files that we may
actually want to be part of module builds.