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

Cleanup proposal concerning ordering of Guidelines and their applicability #493

Open
MarkNahabedian opened this issue Jan 26, 2023 · 0 comments

Comments

@MarkNahabedian
Copy link
Contributor

Motivation:

There are ordering constraints among some Guidelines. For example, guideline_distance_check should be run last. Also, guideline_code_can_be_downloaded should be checked before guideline_version_has_osi_license or any other guideline that depends on the candidate project's source code being available.

This ordering is currently imposed by the two methods of get_automerge_guidelines: one for NewPackage and one for NewVersion. I have verified that these two methods both present the guidelines they do present in the same order. Having two separate implementations of the ordering offers the risk that those orderings could accidentally differ in the future.

In addition to imposing an ordering, get_automerge_guidelines also tests whether a given Guideline is applicable based implicitly on the new package verses new version distinction, and explicitly on conditions hard coded in those methods rather than encoded as part of the Guideline. These conditions are hard coded in each method -- another opportunutry for assidental inconsistency.

Guidelines have various parameters which control, for example, how strict they should be. These parameters are passed in from the client workflows as keyword arguments to RegistryCI.AutoMerge.run. Currently, each of these must be plumbed through run, pull_request_build, and check! to the Guidelines themselves. This is needlessly cumbersome. It looks like in the past, parameters for new Guidelines have been either passed directly as new arguments to these functions, or included in GitHubAutoMergeData.

Perhaps GitHubAutoMergeData should only contain values passed to and returned from the GitHub API. Isolating the GitHub API interactioon to a single data structure is necessary, but not sufficient to address issues like #438 to use RegistryCI with non-GitHub workflows.

Guideline parameters should be passed cleanly, without modification from run, down through to their respective Guidelines. #492 introduces a Dict for this purpose. Perhaps this should be an immutable struct instead? Then we could be sure it doesn't change, but a new field would need to be added for each Guideline parameter.

Proposal:

  1. The constructor for struct Guideline should append the Guideline to global variable ALL_GUIDELINES. This is implemented in Check that there are "enough" tests and documentation #492.

  2. Guideline gets a slot whose value is a collection of types for which the Guideline is applicable (NewPackage, NewVersion, or both).

  3. Guideline gets a new slot whose value is a function that determines Guideline applicability.

  4. Any data needed by get_automerge_guidelines should be passed in through GitHubAutoMergeData or the Guideline parameters Dict rather than as ad-hoc arguments.

  5. A single method of get_automerge_guidelines should loop over ALL_GUIDELINES testing which are applicable based on 2 and 3 above.

  6. Move the definition of each guideline to a separate file named by its global variable name, in a guidelines subdirectory. Include guideline files in the order that they should be checked to control the order they appear in ALL_GUIDELINES.

Open Issues

How would we control when update_status should be called? Currently a special token appears in the Guideline orderings returned by get_automerge_guidelines. I don't understand the motivation for this.

Perhaps each Guideline could have a flag that determines if update_status should be called once the Guideline is checked.

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