Closed ElijahLynn closed 2 years ago
ElijahLynn
commented
4 years ago I have this working locally, and we have 1 warning (https://github.com/department-of-veterans-affairs/va.gov-cms/issues/2870) and 1 error (https://github.com/department-of-veterans-affairs/va.gov-cms/issues/2570) that will fail this new test I am adding, which now I am thinking I should actually push up so we can see it fail. Then once we fix the above, update the branch with a merge of HEAD to see the test then pass.
ElijahLynn
commented
4 years ago Pushed in https://github.com/department-of-veterans-affairs/va.gov-cms/pull/3148. #2870 was fixed a while ago, I just had an outdated local.
Working on fixing #2570 now.
ElijahLynn
commented
4 years ago K, this is working now in https://github.com/department-of-veterans-affairs/va.gov-cms/pull/3148 but is blocked and can't be merged until https://github.com/department-of-veterans-affairs/va.gov-cms/issues/2570 is in otherwise it will start failing all the builds.
cmaeng
commented
3 years ago Work is complete but since implementation is blocked by 2570 our recommendation is to remove as a sprint goal and wait until Tugboat is live to revisit this.
oksana-c
commented
3 years ago @ElijahLynn , I checked the comment thread here and it looks like we can implement now since Tugboat is up and running. This test would be a valuable addition in our D9 effort. Let's 16th min this on Monday.
ElijahLynn
commented
3 years ago I moved this to Sprint 28, not sure if we committed to it in 27, but we didn't get to it this sprint. Could possibly get to it tomorrow still.
ElijahLynn
commented
3 years ago Ensure GitHub repository settings are updated to require the new commit status checks above and that they are not optional
I kinda wonder if we should make the "warnings" test optional in GH. Could be some stuff held up in certain cases by a WARNING that isn't functionally blocking anything.
ElijahLynn
commented
3 years ago Created two new issues that are blockers on merging this in https://github.com/department-of-veterans-affairs/va.gov-cms/pull/3148, because the test is working and we have current PHP Error failures:
ElijahLynn
commented
3 years ago Discussed as team on scrum just now, pausing on this because Drupal throws an Exception on 403s, which sometimes we want to trigger and test for so there will be false positives here. Need a better approach here, and possibly creating an ignorelist for the test was discussed and also making the test not-required (to merge, will still show failed), and using ReviewDog to at least post comments to the issue with the actual warning/error.
ElijahLynn
commented
3 years ago I just tested 403 on DEV and a PHP Warning/Error is not thrown. The only thing in the logs is an "access_denied" type:
Path: /admin. Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException: The 'access administration pages' permission is required. in Drupal\Core\Routing\AccessAwareRouter->checkAccess() (line 117 of /var/www/cms/docroot/core/lib/Drupal/Core/Routing/AccessAwareRouter.php).The Exception we saw on scrum (if I would have scrolled right) was legit and was (https://github.com/department-of-veterans-affairs/va.gov-cms/issues/4676):
12/Mar 22:46 php Error Symfony\Component\HttpKernel\Exception\UnauthorizedHttpException: No ┃ authentication credentials provided. in Drupal\basic_auth\Authentication\Provider\BasicAuth->challengeException()We moved back to unrefined to discuss again in upcoming refinement meeting.
Background
As an engineer, I wish code that introduces new errors to be blocked from merging, so that I do not inadvertently introduce regressions to production that cause WSODs for editors.
Acceptance Criteria
Originally posted by @ElijahLynn in https://github.com/department-of-veterans-affairs/va.gov-cms/issues/2556#issuecomment-682228946