Skip to content

Conversation

@sprankhub
Copy link
Member

@sprankhub sprankhub commented Aug 30, 2019

Description (*)

The following two DevDocs posts nicely show how to use Grunt in order to generate the CSS etc.:

When working on a Magento 2 project, it makes sense that anyone working on the project uses the same Grunt configuration, so that the CSS is generated in the exact same way. Hence, it makes sense to include Grunt configuration files into git. The file /dev/tools/grunt/configs/local-themes.js has already been removed from .gitignore in other pull requests (#18960 / #19350). This pull requests removes the remaining Grunt configuration files from the default .gitignore.

Manual testing scenarios (*)

  1. Create Magento 2 project.
  2. Install and configure Grunt as described in the DevDocs.
  3. Expected behaviour: Grunt configuration files will be added to git.
  4. Actual behaviour: Grunt configuration files are not added to git, because they are included in the default .gitignore.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Aug 30, 2019

Hi @sprankhub. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@torhoehn torhoehn self-requested a review August 30, 2019 08:15
torhoehn
torhoehn previously approved these changes Aug 30, 2019
Copy link
Contributor

@torhoehn torhoehn left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@magento-engcom-team
Copy link
Contributor

Hi @torhoehn, thank you for the review.
ENGCOM-5740 has been created to process this Pull Request
✳️ @torhoehn, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@engcom-Delta
Copy link
Contributor

✔️ QA passed

Copy link
Contributor

@VladimirZaets VladimirZaets left a comment

Choose a reason for hiding this comment

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

 Hi @sprankhub. Thanks for collaboration.
This fix is not fully valid. We provide the sample of Gruntfile.js file and package.json, but we don't want to commit it because of users can have their own customization. If you want to add some customization to all members of your team you can just update sample file that will be successfully committed and all team members will have ability yo use it

@torhoehn
Copy link
Contributor

torhoehn commented Sep 3, 2019

@VladimirZaets I think @sprankhub uses the correct way to share the config files. If they follow your suggestion their changes could be gone with the next update of Magento. So his approach will exactly allow to have own customization and even to share it.

@hostep
Copy link
Contributor

hostep commented Sep 3, 2019

@VladimirZaets: I agree and this problem could be solved if those lines get removed from the .gitignore file which is put in place when running composer create-project --repository-url=https://repo.magento.com/ magento/project-community-edition

That file seems to be slightly different then the one in this repo since it is lacking the line app/etc/config.php (based on a vanilla 2.3.2 install vs https://github.com/magento/magento2/blob/2.3.2/.gitignore)

So how is that .gitignore file changed? Does that file exists in this repo? Or is it somewhere in some non-public repo? Any chance that repo can get opened so people can also contribute to it?

Thanks!

@torhoehn: I think @VladimirZaets means that Magento devs cloning this repo are also using their own version of those files and they don't want to accidentally commit those files, hence why they are ignored.

@sprankhub
Copy link
Member Author

I do not really have anything to add besides the points mentioned by @torhoehn and @hostep. I really think this is the right way. If you are working on a client project, it is vital that any dev can checkout the project and directly work on it without having to copy numerous config files from one place to another.

@sidolov sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:20
@sprankhub
Copy link
Member Author

@VladimirZaets could you have a look at this pull request again given the feedback from @torhoehn, @hostep and me? Thanks!

@ihor-sviziev ihor-sviziev self-assigned this Mar 31, 2020
@ihor-sviziev
Copy link
Contributor

@magento run all tests

@ihor-sviziev
Copy link
Contributor

@sidolov any thoughts?

@sprankhub
Copy link
Member Author

I see "progress needs update". Anything from my side that can be done to push this forward?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Dec 19, 2020

Maybe @gabrieldagama @sivaschenko can help ?

@ihor-sviziev
Copy link
Contributor

maybe @okorshenko ?

@sivaschenko
Copy link
Member

@ihor-sviziev @sprankhub this pull request requires technical discussion and approval from our side as there are different points of view regarding the .gitignore content that should be considered.

Since the priority on this pull request was set to P4, I don't think we'll be able to be addressed soon.

Is there any workaround that can solve this issue for developers that require that without the change for everyone? (I.e. using composer post install script)

Do you think the priority should be increased?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jan 5, 2021

As we discussed in Slack with @sivaschenko:
I don't think we should increase priority for this PR.
I have a feeling that processing (with all needed discussions) of this PR will take just more time than it will bring some value.

@sprankhub what do you think about adding information to the devdocs that in case if you want to add this file to your own git repo - you might need to adjust .gitignore file? What do you think?

@hostep
Copy link
Contributor

hostep commented Jan 5, 2021

If the repository/scripts for creating the magento/magento2-base package and/or the meta package magento/project-community-edition would be open sourced, we could start creating PR's over there. Since the problem can only be fixed in there I believe. But that's probably never going to happen ... 🙁

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jan 5, 2021 via email

@sprankhub
Copy link
Member Author

I still think this change makes sense - especially for client projects. If you would not like to have it in the general repository, it would indeed make sense to implement it somehow in the repository, which creates the meta packages as suggested by @hostep. However, I do not have access to it.

Since adding the respective lines to the .gitignore is kind of obvious and straightforward, I do not think that this needs to be added to the docs. As a reference, this is what we usually append to the default .gitignore from Magento:

!/Gruntfile.js
!/package.json
!/grunt-config.json

Feel free to close this PR if you do not want to / "cannot" merge.

@sivaschenko
Copy link
Member

Hi @sprankhub thank you for the pull request! However, considering there are different opinions about the desired content of gitignore file and estimated low priority/severity of this change I have to close this PR.

@m2-assistant
Copy link

m2-assistant bot commented Jan 11, 2021

Hi @sprankhub, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@sprankhub sprankhub deleted the grunt-config-git branch June 18, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority: P4 No current plan to fix. Fixing can be deferred as a logical part of more important work. Release Line: 2.4 Risk: low Severity: S4 Affects aesthetics, professional look and feel, “quality” or “usability”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants