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

Cd/forceignore bug #1093

Merged
merged 4 commits into from
Sep 18, 2023
Merged

Cd/forceignore bug #1093

merged 4 commits into from
Sep 18, 2023

Conversation

cristiand391
Copy link
Member

@cristiand391 cristiand391 commented Sep 1, 2023

What does this PR do?

Updates the metadata resolver to skip the first forceignore check when resolving components recursively if the dir is a folder.

in the linked GH issue below, when building the component set from the API response SDR is trying checking if unpackaged should be ignored here:

if (this.forceIgnore?.denies(dir)) {

forceignore.denies will pass a relative path to node-ignore that doesn't have a trailing slash so it is treated as a file when matching the forceignore rules against it:

return this.parser.ignores(relative(this.forceIgnoreDirectory, fsPath));

What issues does this PR fix or reference?

forcedotcom/cli#2404
@W-13948748@

@cristiand391 cristiand391 requested a review from a team as a code owner September 1, 2023 17:08
// `forceignore.denies` will pass a relative path to node-ignore, e.g.
// `path/to/force-app` -> `force-app`, note that there's no trailing slash
// so node-ignore will treat it as a file.
if (!this.tree.isDirectory(dir) && this.forceIgnore?.denies(dir)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@mshanemc it turns out the bug also affects a real FS tree:
Screenshot 2023-09-01 at 18 51 35

path.relative doesn't return a trailing slash so node-ignore will treat as a file, called here:
https://github.com/forcedotcom/source-deploy-retrieve/blob/f288319e55378d1a7caca5bcd762b77b2c59f30a/src/resolve/forceIgnore.ts#L66C1-L66C1

another option I showed you this morning was to add the trailing slash before passing dir to denies, but then have to remove it again or it will not be able to file the folder in the tree:

for (const file of this.tree.readDirectory(dir)) {

that also worked, but not sure if it's the right solution:
https://stackoverflow.com/questions/980255/should-a-directory-path-variable-end-with-a-trailing-slash

@mshanemc
Copy link
Contributor

mshanemc commented Sep 18, 2023

QA notes: using dreamhouse and project:deploy:preview

* ignores everything
!*/ allows all non-directories (everything is still ignored)
!**/classes/** makes all classes show up as "will deploy"

created class Foo in the org via the (eww) dev console per repro.

project retrieve preview shows that it will be retrieved
project retrieve start retrieves the classes and saves them locally to the expected location

Now, let's try to break it

  • remove the * options from the forceignore so that the whole project deploys
  • change the display test in barcodeScanner.html (lwc)
    deploy preview shows correct (will deploy lwc)
    * to ignore everything => throws Error (1): The "path" argument must be of type string. Received undefined

(looks like this is broken in main, not necessarily a bug in this PR)

Note: if you put this back in the .forceignore, it produces the expected results (on both main and this PR)

# First, ignore everything
*

# Now, allow anything that's a directory
!*/

# Then allow anything in the classes directory
!**/lwc/**

@mshanemc mshanemc merged commit 8c619a8 into main Sep 18, 2023
65 of 66 checks passed
@mshanemc mshanemc deleted the cd/forceignore-bug branch September 18, 2023 18:46
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