-
Notifications
You must be signed in to change notification settings - Fork 9.4k
[vcl] Better debug #28944
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
[vcl] Better debug #28944
Conversation
Hi @gquintard. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. |
@magento run all tests |
@magento run Unit Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ Thank you for your expertise!
Hi @lbajsarowicz, thank you for the review.
|
@magento run Integration Tests, Functional Tests CE |
@lbajsarowicz, apologies for my ignorance, is there anything I should do here? |
@gquintard No. Now we wait for the process of the merge to be performed. |
Automated Tests are not applicable, as the change is related to infrastructure configuration. |
Dev experience is required for testing of this PR. Please note that Manual testing has not been performed. |
@magento run all tests |
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. |
Hello @gquintard |
hi @andrewbess, sure. Can I do that once we are just about to merge? I'll rebase an squash at that time. |
Yes, of course |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fixes look nice for me
Approved
@magento run Functional Tests EE, Functional Tests B2B |
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. |
@magento run Functional Tests B2B |
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. |
@magento run Functional Tests B2B |
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. |
@magento run Functional Tests B2B |
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. |
The failed B2B test seems flaky to me. Please refer to the below screenshots: |
We've established earlier that the VCL code changes aren't taken into account by automated tests (the test used the system-wide VCL which doesn't match the in-repository code). I do believe it's an issue in itself, but I'm fairly confident the flaky tests aren't caused by this change. |
Detecting if the response is a hit usingthe
x-varnish
header is flakyand notably doesn't work in a multi-tier setup.
On top of this, HIT/MISS is a false dichotomy (https://info.varnish-software.com/blog/using-obj-hits)
and while I'd like something more detailed like
https://docs.varnish-software.com/tutorials/hit-miss-logging/ (tech
version of the blog), I recognize that it's "a lot" of VCL, so this
commit only differentiates between hits, misses and passes, the latter
being pretty useful to identify while debugging.
note: all versions have been changed for the sake of consistency
but both the 4.x and 5.x series have been EOL'd a (long) while ago and users
should be encouraged to upgraded as soon as possible.
edit: I've added a test that you can run with
varnishtest dev/tests/varnish/*.vtc
since the currentphp
suite doesn't correctly test the committed VCL. It would probably be smart to run that command as part of the test suite, but I'll leave that to more knowledgeable people than myself.Contribution checklist (*)
Resolved issues: