Refactor remote_development policy debugging
MR: Pending
Description
We have the debug_policies
helper method in all our remote_development policy specs.
Currently, this must be turned on by setting the flag debug = true
e.g https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/spec/policies/remote_development/workspace_policy_spec.rb?ref_type=heads#L42
However, in this thread (!179304 (comment 2343802078)) it was raised that it is not a good practice to require developers to edit the source to enable debugging, and it was proposed that we should instead use ENV variables to enable this debugger.
This is a good idea, and that is the purpose of this issue.
However, we want to achieve this via a new ENV variable and not reuse the existing GITLAB_DEBUG_POLICIES
, because that represent a completely different debugging mechanism and output than these custom methods.
References for this decision:
So, we should introduce a new environment variable for this purpose. Perhaps GITLAB_DEBUG_POLICIES_VERBOSE
or GITLAB_DEBUG_SPECIFIC_POLICIES
or GITLAB_CUSTOM_DEBUG_POLICIES
. Whatever works, as long as it isn't reusing the existing GITLAB_DEBUG_POLICIES
variable.
We probably don't want to use the domain name (WORKSPACES_
) as part of the ENV var, because we want this pattern to be reusable (with common documentation) across different domains/bounded contexts.
Acceptance Criteria
-
Remove the debug
flag from the policy spec files for remote_development -
Enable the debug_policies
helper when the ENV variable is set to any truthy value. -
Use Gitlab::Utils.to_boolean
to support the truthiness-checking logic.
Technical Requirements
Same as acceptance criteria
Previous context on issue
This issue was previously created as a followup to this thread.
The concern was raised whether this approach of allowing custom debugging methods per spec file is maintainable in the long run and a better solution would involve a generalized helper that may be re-used in different policy specs.
However, as we have continued to implement new policy files, we have further validation of the usefulness of these inline, policy-specific debug methods. Just within the past week, the output of these methods has been used and shared when discussing bugs/mysteries in new DeclarativePolicy-related code.
We have also clearly documented this approach and the motivation behind it: https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/lib/remote_development/README.md#guidelines-for-writing-tests-using-this-pattern. This documentation, along with likely many other topics in that README which could be useful across multiple domains, can and should be promoted to the main documentation site. We already have issues for that (TODO: find link).
So, until there is any compelling reason to remove them or try to DRY them up (likely not useful or desirable), we will leave them inline.
Furthermore, there is the context of the recent decisions (from the Engineering Tech Summit) to use the Category:Workspaces bounded context as an initial exploratory case of modularization. This will likely see the entirety of the Workspaces domain code moved to a dedicated subdirectory. Thus, there is arguably less of a concern about enforcing consistency, especially when it involves proposals to change patterns which are proven useful to the the group responsible for maintaining that domain.
However, these concerns should still certainly be carefully considered and discussed, and there's already a separate issue dedicated to exactly that topic: Evaluate how the modular monolith might impact ... (#462955)
Therefore, this issue is now focused solely on the improvement of controlling the debug behavior via an ENV var rather than requiring the editing of a local variable.