-
Notifications
You must be signed in to change notification settings - Fork 244
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
Fix ignores files during sync #5689
Fix ignores files during sync #5689
Conversation
✅ Deploy Preview for odo-docusaurus-preview canceled.
|
Kudos, SonarCloud Quality Gate passed!
|
I noticed that while the changes here worked great for sync'ing, we were still unnecessarily watching files that were supposed to be ignored. For example, I saw these logs while testing on a simple multi-module Maven project:
As discussed together, the "watch" part will be fixed in a separate issue/PR. LGTM so far, but I'll let other reviewers (@dharmit , @valaparthvi ) review and approve. /lgtm EDIT: Created #5698 to keep track of this |
I tried a scenario where $ echo -e "\nb.txt" >> .gitignore
$ echo hello > b.txt The |
Is the b.txt synced to the container? As noted by @rm3l, this PR fixes the files that are synced to the container, but not particularly which files are triggering the sync. |
Yes. $ kubectl exec nodejs-app-9964cc475-94ct6 -c runtime -- cat /projects/b.txt
hello
OK, this makes a lot more sense now.
I don't know/remember how it behaved in the past. But I think it was buggier in the past than what you are proposing in this PR. So, maybe, we should let this in and see if the scenario I discussed is a problem to someone for real. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dharmit The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
After some more tests, I think that modifying the .gitignore file is not supported during a dev session: the initial content will be used during all the session. |
I remember commenting about this issue in this other PR about |
/override ci/prow/v4.10-integration-e2e |
@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.10-integration-e2e In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/override ci/prow/unit |
@feloy: Overrode contexts on behalf of feloy: ci/prow/unit In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
* fix ignore files * Add tests for synced files * Use go-gitignore library * rename topPath
What type of PR is this:
/kind bug
What does this PR do / why we need it:
Which issue(s) this PR fixes:
Fixes #4389
PR acceptance criteria:
Unit test
Integration test
Documentation
How to test changes / Special notes to the reviewer: