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

bug: watch command deploys unmodified files that match a modified files filename #138

Open
techfg opened this issue Jun 25, 2024 · 0 comments

Comments

@techfg
Copy link

techfg commented Jun 25, 2024

Per the docs, the watch command should only ever deploy modified files, however in some cases, it will deploy files that have not changed locally on disk.

As mentioned in #128, there is a current issue where watch will deploy any file in the same directory as a modified file that starts with the same character as the modified file. However, even with that issue addressed (see #137), there is another issue in the logic that determines which files to deploy when a file is modified. This will occur in either of the following situations:

  1. there are multiple files in the same directory as the modified file that "start with" the same characters (e.g., modifying the page bill.xml will result in both bill.xml and billy.xml being deployed).
  2. Modifying any file in the "site" directory will result in all files in "sitepermissionsets" being deployed

Both #128 and the issue described here are critical flaws in the way watch behaves and could lead to unexpected results for which the user isn't even aware that a file(s) was deployed as standard usage of skuid-cli doesn't emit the filenames deployed when changes are detected. In short, the user would have to have diagnostic and/or trace enabled AND be paying very close attention to the logs emitted to even be aware that unmodified files were deployed.

For example, if the files locally on disk are stale/old and a file is modified intentionally, if any of the old files are deployed, it could overwrite more recent updates to the files/metadata in the target site - all potentially unbeknownst to the individual modifying the file AND potentially against a production system (if that is the target site).

Steps to reproduce

  1. Clone repo which contains a fix for bug: watch deploys all files in modified file directory that start with same character as modified file #128 so that the issue described here is more apparent
  2. Checkout fix/resolve-watch-issues branch
  3. Execute go build
  4. Create a new NLX site or use an existing
  5. Create two pages with the names bill, billy (no need to put anything in the page)
  6. Execute skuid retrieve -d skuid-objects using the binary from step 3
  7. Execute skuid watch -d skuid-objects --trace using the binary from step 3
  8. Make a change to pages/bill.xml and save the file

Expected Result
Only pages/bill.xml and its corresponding pages/bill.json should be deployed

Actual Result
The xml & json files for BOTH bill and billy in the pages directory are all deployed.

Relevant Log Entries

  1. Note the pages/bill in the first line as expected
  2. Note 4 files processed instead of 2 (2 for bill & 2 for billy)
...
DEBU[0005] Detected change to metadata type: pages/bill
...
TRAC[0004] Finished Processing pages/billy.xml
TRAC[0004] Finished Processing pages/bill.xml
TRAC[0004] Finished Processing pages/billy.json
TRAC[0004] Finished Processing pages/bill.json

Steps to reproduce (continued)

  1. Make a change to site/site.json and save the file

Expected Result
In theory, only the file site/site.json should be deployed but if necessary for Skuid to deploy properly, at most, all the files in site/ directory are deployed.

Actual Result
All files in site/ and sitepermissionsets/ are deployed

Relevant Log Entries

TRAC[0003] Finished Processing site/favicon/favicon.ico 
TRAC[0003] Finished Processing site/logo/APR_Primary_Gradient.png 
TRAC[0003] Finished Processing sitepermissionsets/AppAdmin.json 
TRAC[0003] Finished Processing site/logo/APR_Primary_Gradient.png.skuid.json 
TRAC[0003] Finished Processing site/site.json  
TRAC[0003] Finished Processing sitepermissionsets/Admin.json 
TRAC[0003] Finished Processing sitepermissionsets/External.json 
TRAC[0003] Finished Processing sitepermissionsets/External_Builder_Edit.json 
TRAC[0003] Finished Processing sitepermissionsets/External_Builder_Inspection.json 
TRAC[0003] Finished Processing sitepermissionsets/External_Developer_Edit.json 
TRAC[0003] Finished Processing sitepermissionsets/External_Developer_Read_Only.json 
TRAC[0003] Finished Processing site/favicon/favicon.ico.skuid.json 
TRAC[0003] Finished Processing sitepermissionsets/Inspector.json 
TRAC[0003] Finished Processing sitepermissionsets/Manager.json 
TRAC[0003] Finished Processing sitepermissionsets/Public.json 
TRAC[0003] Finished Processing sitepermissionsets/Standard.json 

Additional Information

  1. The issue is occurring because of how the value for changedEntity is determined and the way ArchivePartial behaves. In short, the approach uses the base name of the file (not the full filename) and performs a starts with (prefix) compare against all files on disk (in the case of "site" files, the word "site" is used as the prefix and hence why "sitepermissionsets" are also deployed). This results in false positives being detected when files start with the same characters (e.g. bill & billy, site & sitepermissionsets). I believe it takes this approach to ensure that related/associated metadata files for the modified file are also deployed (e.g. pages/bill.xml & pages/bill.json both need to deploy whenever one or the other is modified), however it has the negative consequence of deploying files that have not changed.
  2. Note that resolution of the base name of the file itself isn't even foolproof as it simply takes anything to the left of the first period (.) as the base name. For situations where an entity has a period in its name (e.g., files/my.javascript.file.js) this would result in anything matching files/my* getting deployed and a reason to move away from "starts with" approach.
  3. PR fix: resolve several issues with watch command #137 addresses the issues in bug: watch deploys all files in modified file directory that start with same character as modified file #128 but does not address the problems described in this issue as it would require a much more invasive approach to resolve. Based on what I'm seeing in the code, rather than "starts with" detection, there likely needs to be detection of the "metadata type" and then specific logic against the full filename of the modified file to include any associated/related files that must be included in the payload for the modified file (e.g. pages/bill.xml would be detected as a page and then pages/bill.json specifically included because its known a page has a json file). As there isn't any public documentation of all the different metadata types and files associated with each "entity" and without a full test suite, this is best left for internal Skuid team to identify best approach and address. With that said and as mentioned above, I believe this should be considered a critical bug which could lead to completely unexpected behavior against a potentially production system without the user even knowing it and therefore should be addressed ASAP.
techfg added a commit to techfg/skuid-cli that referenced this issue Jun 27, 2024
Resolves skuid#138
Addresses the fatal termination portion of 141 & 142
@techfg techfg changed the title watch command deploys unmodified files that match a modified files filename bug: watch command deploys unmodified files that match a modified files filename Jul 23, 2024
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

No branches or pull requests

1 participant