[vcl] don't manually interfere with compression

[gquintard]

Description (*)

Varnish already has default compression handling. This code only kicks into gear in case of a miss or a pass, and in those cases, the backend should be responsible for these.

Manual testing scenarios (*)

automated testing should do the job

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)
• [ ] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• [ ] All automated tests passed successfully (all builds are green)

Resolved issues:

1. [x] resolves magento/magento2#38309: [vcl] don't manually interfere with compression

[m2-assistant[bot]]

Hi @gquintard. Thank you for your contribution! Here are some useful tips on 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.4-develop instance - deploy vanilla Magento instance

:exclamation: Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here :information_source: Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation. Join Magento Community Engineering Slack and ask your questions in #github channel.

[hostep]

This was also proposed in https://github.com/magento/magento2/pull/36796, but since it lacked feedback from Adobe people it got closed.

Maybe taking things in smaller steps will have a bigger chance of being accepted (but knowing Adobe, it will still take months if not years 🙁 )

[ihor-sviziev]

@magento run all tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Echo]

@magento create issue

[engcom-Hotel]

@magento run Functional Tests B2B

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

Hello @gquintard,

Thanks for the contribution!

We need a testing scenario to check this PR manually. Please provide us with the same.

Meanwhile we are moving this PR On Hold.

Thanks

[gquintard]

Happy new year, I've just pushed a test, using the same logic as in #28944, I omitted the README to avoid conflicts, hoping the other PR will land before.

[engcom-Hotel]

@magento run all tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

Hello @gquintard,

As the changes happened in the PR moving back this PR into review.

Thanks

[engcom-Hotel]

@magento run all tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Echo]

@gquintard can you please provide manual step for reproduction of the issue.

[gquintard]

hi @engcom-Delta, there's no bug per se, but the current code is redundant and can be taken out without changing behavior.

You can run the added tests using varnishtest dev/tests/varnish/*.vtc

[engcom-Hotel]

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Unit Tests, WebAPI Tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[gquintard]

well, that's unexpected: pwd=<PATH>38211 What kind of platform are you running this on?

the "extra characters at the end of h command" error message makes no sense to me at the moment but I expect the > and < in the path are messing us up, so I did something very stupid to debug: I replicated the command multiple time, adding more and more arguments with every line, to see which argument causes the problem. If you could grab the latest version and share the new logs, it would be great.

(I've also quoted the path to avoid more issue, and it could solve the problem entirely, I'm unsure)

[engcom-Hotel]

@gquintard Thanks for the quick workaround!

We are using Mac OS and set up the instance in our local machine.

After taking the latest pull, the test is still failing. Please refer to the updated log below:

test.log

We have just replaced our folder path with /DummyPath/ in the log file.

After digging into the compression.vtc file, I found that we need to pass an empty quote after the sed command for MAC machines. I added the same in the vtc file, but it shows failed. Please refer to the latest log file below:

test.log

Please let us know if we missed anything.

Thanks

[gquintard]

@engcom-Hotel , thank you for your patience. I was able to get access to a mac thanks to the awesome @briiians, and it looks like on it, sed FILE -e EXPRESSION doesn't work (FILE is interpreted as an expression).

So, normally, this should be fixed, please have another look

[engcom-Hotel]

@magento run all tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

@magento run Functional Tests B2B

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

:heavy_check_mark: QA Passed

Varnish already has default compression handling. This code only kicks into gear in case of a miss or a pass, and in those cases, the backend should be responsible for these.

Preconditions:

Varnish should be enabled as Full Page Cache

Manual testing scenario:

Run the below command to check the test result: varnishtest dev/tests/varnish/compression.vtc

Actual Result: :heavy_check_mark: The test should pass:

This PR is just an improvement.

Tested all the manual scenarios, no impact on regression testing.