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

Ignore .eslintrc.json at project init #548

Closed
nvuillam opened this issue Mar 30, 2023 · 24 comments · Fixed by #554 or #562
Closed

Ignore .eslintrc.json at project init #548

nvuillam opened this issue Mar 30, 2023 · 24 comments · Fixed by #554 or #562
Assignees
Labels
enhancement New feature or request

Comments

@nvuillam
Copy link
Contributor

When we do an init of CI project with sfdx-hardis, the first package.xml is computed using the great sfdx-git-delta :)

But it always adds .eslintrc.json in Aura and LWC types... so we have to update it manually

Would it be possible to always ignore them, as they won't never have any use being deployed to an org ? :)

@nvuillam nvuillam added the bug Something isn't working label Mar 30, 2023
@scolladon
Copy link
Owner

scolladon commented Mar 31, 2023

Hi @nvuillam!

Thanks for raising this issue and thanks for contributing in making this project better!

I think you could use the --ignore parameter of the plugin, it looks like to be exactly what you need.
You could feed this parameter with the .forceignore or the .gitignore if their content exclude those files

Let us know if it is suited for your use case, if not let's talk about a new feature!

@scolladon scolladon changed the title [BUG NAME] Ignore .eslintrc.json Ignore .eslintrc.json at init Mar 31, 2023
@scolladon scolladon changed the title Ignore .eslintrc.json at init Ignore .eslintrc.json at project init Mar 31, 2023
@scolladon scolladon changed the title Ignore .eslintrc.json at project init Ignore .eslintrc.json at project init Mar 31, 2023
@nvuillam
Copy link
Contributor Author

nvuillam commented Mar 31, 2023

Thanks for the reply :)

But I really think it should be ignored by default because such behaviour is corresponding to 150% of sfdx-git-delta use cases, and these files must be committed anyway (and they are already in .forceignore by default)

@scolladon scolladon added enhancement New feature or request and removed bug Something isn't working labels Mar 31, 2023
@scolladon
Copy link
Owner

Hi @nvuillam, thanks for taking some times discussing this together in dm, really appreciated
From a pure product standpoint we aim to provide a flexible and simple plugin, we do not know better than our customers and do not pretend we could foresee every use cases 😅.
This is why we provide optional parameters to help customer drive the plugin to achieve their particular goals.

Framing the requirement

In this situation the need as I understand it is to not add in the package.xml the elements that are not metadata (indeed makes sense...).
Question can be asked as well for the output folder containing incremental changes. Maybe there are uses cases where we want them to be copied anyway (splitting/revamping project per example), so let's consider this only at the package.xml level
it seems to be possible to consider using --ignore parameter as a workaround to not include those files.
And it seems not necessary to use sgd at the init as the whole repository content will be deployed anyway, unless we want to use sgd to generate the init package.xml (which is smart).

Solution space

Here are the solution I can imagine for now (by order of preferences):

  1. improve the plugin to not add in the package.xml element that are not metadata compliant (generic solution)
  2. have a default .sgdignore used by the plugin when -i is not used (use it instead when filled) (configurable solution that would require maintenance to black list new elements)

Let's think/spike on that, maybe more solution will raises and maybe more people will need it or explain something similar which will help us shape another solution !

@mehdicherf
Copy link
Collaborator

mehdicherf commented Mar 31, 2023

@scolladon My 2 cents:
IMO it's a minor issue since there is a workaround (--ignore), and it does not happen often (only once, when you initiate the repo). Still, in my view, it's an issue, as it not expected to have element that are not metadata compliant in the package.xml.

I don't have a definitive opinion as whether it should be in the output folder containing incremental changes. But for sure it should not be in the package.xml.
Meaning that we should not have this kind of package.xml generated by SGD, as it's not something we would deploy:

<?xml version="1.0" encoding="UTF-8"?>
<Package xmlns="http://soap.sforce.com/2006/04/metadata">
    <types>
        <members>.eslintrc</members>
        <name>LightningComponentBundle</name>
    </types>
    <version>57.0</version>
</Package>

So, I would vote solution # 1!

@nvuillam
Copy link
Contributor Author

@scolladon as discussed, I also vote Solution 1 : avoid items that are not metadata in package.xml, without having to add extra parameters to the command :)

@AllanOricil
Copy link
Contributor

I couldn't reproduce this. I ran sgd without passing a .sgdignore in a brand new sfdx project, and nothing was added in the package.xml. Then I created a dummy lwc, added a new commit, ran sgd again, and finally verified that only the lwc was included. Then I deleted .forceignore, repeated the previous steps, and again verified only the lwc was included in the package.xml.

@AllanOricil
Copy link
Contributor

The same for aura. No .eslintrc. was inlcuded in the package.xml that sgd created

@AllanOricil
Copy link
Contributor

I tested with 5.13.3 and 5.15.0

@AllanOricil
Copy link
Contributor

more or less related #552

@nvuillam
Copy link
Contributor Author

nvuillam commented Mar 31, 2023

I didn't invent the issue ;)
But it happens only when you init a branch and when there is nothing in the master branch ^^
(Quite unconventional use but really useful for sfdx-hardis CI/CD projects :) )

@scolladon
Copy link
Owner

Hi @nvuillam

I just finalized a first draft of a PR (#554) to implement the solution 1.
If you could have a look at it and do the QA on your end and let us know your result (here or in the PR) it would be greatly appreciated!
Here are high level steps to do it.
(do not hesitate to contact me if those steps are not clear enough or if you have any blockers while doing those)

@nvuillam
Copy link
Contributor Author

nvuillam commented Apr 1, 2023

@scolladon of course :)
I'll just clone locally your branch, sfdx plugins:link and check with one of my use cases :)

@AllanOricil
Copy link
Contributor

AllanOricil commented Apr 3, 2023

Somehow my previous tests were all wrong. Now that I have a test workbench repo in Github, I can kind see this problem.

image

In my case, Instead of end up having an .eslintrc entry in the package.xml, sgd adds workflow changes that are in the .github folder.
image

@scolladon
Copy link
Owner

Hi @AllanOricil

What is the SGD version running ?
Could you let me know how I can reproduce it locally ?
Can I access the repo you are mentioning and what command should I run to reproduce ?

What is the output of this command:

$ git diff --name-status --no-renames <from> <to>

@AllanOricil
Copy link
Contributor

@scolladon

Im using sgd 5.16.0, but the issue also happens with 5.14.0.

This is the commit history of this repository

image

This is the output of git diff --name-status --no-renames ${{ inputs.FROM }} HEAD

image

@scolladon
Copy link
Owner

Thanks for those information @AllanOricil.

According to the output of the diff command the result is consistent with the actual behavior of the plugin.
We can see it reproduced by the github workflows file present in the package.xml.
As there is no changes detected by git related to .eslintrc.json file it is not processed by the plugin and does not appear in the package.xml.
It is the result of the same behavior internally, the fix shipped will fix both.

It is expected to have the issue with any previous version.
The fix (#554) is going to be shipped in the next release (available first in the latest-rc release channel).
Stay tuned!

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

Shipped in release v5.17.0.
You can install the new version using the version number or the latest-rc channel

$ sfdx plugins:install sfdx-git-delta@latest-rc
$ sfdx plugins:install [email protected]

@nvuillam
Copy link
Contributor Author

nvuillam commented Apr 4, 2023

@scolladon sorry, just tested with @latest-rc and I still have .eslintrc.json :/

(but it's ok for the github workflow not appearing ^^ )

image

@scolladon
Copy link
Owner

scolladon commented Apr 4, 2023

Hi @nvuillam

Thanks for the feedback and for your time testing it.

I'm not able to reproduce it locally
I have a created a dedicated branch in our reproduction playground.

When you check it out:

$ git diff --name-status HEAD~3
A       .forceignore
A       .github/CODEOWNERS
A       .github/workflows/pull-request.yml
A       README.md
A       output/.keep
A       package.json
A       sfdx-project.json
A       sgd/reproduction/playground/aura/.eslintrc.json
A       sgd/reproduction/playground/classes/Test.cls
A       sgd/reproduction/playground/classes/Test.cls-meta.xml
A       sgd/reproduction/playground/lwc/.eslintrc.json

This is what we want to reproduce because it contains eslintrc.json and pull-request.yml github workflow files

When it executes using `v5.17.0``

$ sfdx sgd:source:delta -f HEAD~3 -d

$ cat output/package/package.xml
<?xml version="1.0" encoding="UTF-8"?>
<Package xmlns="http://soap.sforce.com/2006/04/metadata">
    <types>
        <members>Test</members>
        <name>ApexClass</name>
    </types>
    <version>57.0</version>
</Package>

$tree output
output
├── destructiveChanges
│   ├── destructiveChanges.xml
│   └── package.xml
├── package
│   └── package.xml
└── sgd
    └── reproduction
        └── playground
            └── classes
                ├── Test.cls
                └── Test.cls-meta.xml

The package.xml does not contains eslintrc.json nor pull-request.yml github workflow files

Maybe my setup to reproduce it is wrong.
Could you help me reproduce it @nvuillam ? What is the specificities of your setup ?

@nvuillam
Copy link
Contributor Author

nvuillam commented Apr 4, 2023

@scolladon everything you need is here :)

I also just noticed that the files are .eslintrc and not eslintrc.json, I don't know if it matters ^^

Edit: with the sample repo it's probably better :D https://github.com/nvuillam/git-delta-test

@scolladon
Copy link
Owner

Thank you very much @nvuillam

I cannot reproduce even using the repository provided.
Using v5.16.0 I can see the issue
Using v5.17.0 the issue is fixed

Here is the protocol used:

$ git clone https://github.com/nvuillam/git-delta-test.git
$ cd git-delta-test
$ git fetch -pPt
$ mkdir output
$ sfdx sgd:source:delta --from 59a9d219d7f0b9a89f75bcadb8ea6ca078d6e106 --to 51800a3ff84d52568bb43740c1fa4b39aba65c3d -d
$ cat output/package/package.xml
$ tree output

I suspect the issue is related to the environment (mac vs windows)
I'll try tonight with a Windows VM with git bash and let you

@scolladon
Copy link
Owner

I've been able to reproduce on Windows.
Thanks @nvuillam for your help here.
I'll ship a fix tonight.

Stay tuned !

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

Shipped in release v5.17.1.
You can install the new version using the version number or the latest-rc channel

$ sfdx plugins:install sfdx-git-delta@latest-rc
$ sfdx plugins:install [email protected]

@nvuillam
Copy link
Contributor Author

nvuillam commented Apr 5, 2023

No more .eslintrc , I confirm this is ok with latest-rc, many thanks @scolladon :)

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants