Skip to content
This repository has been archived by the owner on Jun 2, 2023. It is now read-only.

Technical prereqs for big org-wide label sync #60

Merged
merged 5 commits into from May 19, 2023

Conversation

kdmccormick
Copy link
Collaborator

@kdmccormick kdmccormick commented Apr 27, 2023

This is just a technical implementation PR. The list of labels here is not the full list. To see the full list of labels we plan to add, please see: #61

Background

This implements 1. Proposal: Store labels in one central place, following the "Store the labels in a new labels.json labels.yml file in the same directory as repo_checks.py" option.

It also spruces up repo_checks a bit in order to run the check which creates/updates the labels. Notably, we introduce the --check option, and rename --target to --repo.

Next up after this PR would be:

Description

Changes are broken up by commit. See the commit log.

Testing & Applying

I did a dry run of this using: python -m migrate.repo_checks -c ensurelabels.

The beginning of the output, with colors:

image

Full output: https://gist.github.com/kdmccormick/708fc262ac751ca4728a4d7bc208e93b

Only a handful of repos (the very newest ones) would have labels created by this. Some repos will have label descriptions updated. The majority of repos will have no changes.

@kdmccormick kdmccormick changed the title refactor: extract repo_checks labels out to YAML file Get ready for big org-wide label sync Apr 27, 2023
@kdmccormick kdmccormick requested a review from feanil April 27, 2023 20:33
@mphilbrick211
Copy link

Hi @kdmccormick! I noticed we no longer have the "Need tests run" label - Tim and I use it for OSPR triage so it's clear to whoever is running the tests, which PRs need it. Any chance we could get it back? We have a status column for an OSPR that needs a CLA or tests enabled, but without the actual test label, we'd have to click in each OSPR to see if tests needs to be run.

"needs test run" | Author’s first PR to a repository, awaiting test authorization from AximThis means an author has not contributed to a particular repo before and needs authorization to be able to run tests on the PR. Currently, test authorization is handled by Axim.

@kdmccormick
Copy link
Collaborator Author

@mphilbrick211 This specific PR, with its limited set of labels, exists only to address some technical prerequisites to label syncing. It will not remove any of the labels you folks use for OSPRs 😄

We'll be explicitly adding the needs test run label, along with all other current OSPR labels, in this PR: https://github.com/openedx/terraform-github/pull/61/files#diff-6129a865dba373b37efda34d6fa564cad8f4cdb80991ab6ac61b5a2900ffe5deR100-R102 . Please check out that PR if you'd like to see the full list of labels we plan on having available.

@kdmccormick kdmccormick changed the title Get ready for big org-wide label sync Technical prereqs for big org-wide label sync May 3, 2023
@mphilbrick211
Copy link

@mphilbrick211 This specific PR, with its limited set of labels, exists only to address some technical prerequisites to label syncing. It will not remove any of the labels you folks use for OSPRs 😄

We'll be explicitly adding the needs test run label, along with all other current OSPR labels, in this PR: https://github.com/openedx/terraform-github/pull/61/files#diff-6129a865dba373b37efda34d6fa564cad8f4cdb80991ab6ac61b5a2900ffe5deR100-R102 . Please check out that PR if you'd like to see the full list of labels we plan on having available.

Thanks @kdmccormick! The label is not currently available for us to use - do you know when we'll have access to it again?

@itsjeyd
Copy link

itsjeyd commented May 5, 2023

@mphilbrick211 @kdmccormick As of this morning, needs test run is showing up in the list of PR labels for me:

example-from-frontend-app-learning

This is a screenshot from openedx/frontend-app-learning#981.

So it looks like the issue has been fixed or, if you're still seeing it @mphilbrick211, it might be repo-specific?

@kdmccormick
Copy link
Collaborator Author

@mphilbrick211 It is likely that the needs test run label is missing from repos, although I'm not sure exactly why that would happen. The label-syncing situation is pretty complicated right now, which is why I'm making these changes.

I recommend adding it manually for now. I hope to have all the OSPR labels added consistently & automatically by the end of May.

@mphilbrick211
Copy link

@mphilbrick211 It is likely that the needs test run label is missing from repos, although I'm not sure exactly why that would happen. The label-syncing situation is pretty complicated right now, which is why I'm making these changes.

I recommend adding it manually for now. I hope to have all the OSPR labels added consistently & automatically by the end of May.

Thanks, @kdmccormick - though, Tim and I aren't able to create labels.

@mphilbrick211
Copy link

Just saw @itsjeyd's comment - the label appears for me in most of the larger repos, but not in ones like "openedx/openedx-event-sink-clickhouse". So, @kdmccormick it looks like this project will fix that :)

@itsjeyd
Copy link

itsjeyd commented May 8, 2023

@mphilbrick211 I think by "adding it manually", @kdmccormick might have been referring to posting a comment of the form label: needs test run.

I just tested this on openedx-unsupported/openedx-event-sink-clickhouse#2 and it worked fine:

manually-adding-label

--

Side note: This is how I've been adding the core contributor label to relevant PRs.

When handling an HTTP 409 for empty git repos, this:

    e.fp.read()

was yielding an empty bytestring. This caused us to pass through the
empty-repo handling code and crash with the HTTP 409.
Instead, we just do this:

    str(e)

and compare against that output, which is nearly as good.
@kdmccormick kdmccormick force-pushed the kdmccormick/labels branch 2 times, most recently from cbc2b37 to a5ca8b9 Compare May 12, 2023 20:32
The --check/-c option allows the name of a check class to be passed
in, case-insensitively:

    python -m migrate.repo_checks -c requiretriageteamaccess -c requireclacheck

Additionally, we improve the output in a few ways:
* bold the repo names
* make skipped checks cyan to distinguish them from steps
* print out checks that will be run at the beginning
* print check class names instead of class repr's

Finally, we add usage instructions to the module docstring.
We are preparing make the EnsureLabels checks the one-and-only
authoritative place for org-level labels. As part of this, we're
extracting the list of labels out of the Python script and into
a YAML file.

For repos that are already up-to-date with repo_checks, this change
should be a no-op.

Additionally, this cleans up the EnsureLabels code a bit so that it'll
be easier to add label-description support in the next commit.

Part of:
https://openedx.atlassian.net/wiki/spaces/COMM/pages/3695214629/openedx+org-wide+labels#1.-Proposal%3A-Store-labels-in-one-central-place
repo_checks currently normalizes label names and colors. With this
commit, it will also normalize the label description.

In interest of minimizing the effect of running the EnsureLabels check:
For labels that generally have already-established descriptions, we use
those. For labels that don't, we just use !!null for now, with the plan
of adding descriptions in the near future.

Part of:
https://openedx.atlassian.net/wiki/spaces/COMM/pages/3695214629/openedx+org-wide+labels#2.-Proposal%3A-List-of-starter-labels
@kdmccormick kdmccormick merged commit 7e3caa8 into openedx-unsupported:main May 19, 2023
2 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/labels branch May 19, 2023 18:05
kdmccormick added a commit to kdmccormick/terraform-github that referenced this pull request May 19, 2023
Fixes openedx-unsupported#60

I had tested the --dry-run but not the --no-dry-run code path.

Just needed to update the arguments to the create_issue API call.

For good measure, I also update both create_ and update_ API calls
to use only kwargs.
@kdmccormick
Copy link
Collaborator Author

Other than a minor hiccup, fixed by #62, this applied successfully 🚀

kdmccormick added a commit that referenced this pull request May 23, 2023
Fixes #60

I had tested the `--dry-run` but not the `--no-dry-run` code path.

To fix, we just update the arguments to the `create_issue` API call.

For good measure, we update both the `create_issue` and
`update_issue` API calls to use only kwargs.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants