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.

Sign up for GitHub

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

Jump to bottom

improve repo validation error message #1764

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

improve repo validation error message #1764

wants to merge 1 commit into from

Conversation

marcoieni
Copy link
Member

@marcoieni marcoieni commented Apr 4, 2025 •
edited
Loading

A user complained in #1763 (comment) that this error was confusing.

tested against that PR:

$ GITHUB_TOKEN=$(gh auth token) rust-team check                                                                  
[WARN  rust_team::validate] couldn't perform checks relying on the Zulip API, some errors will not be detected
[WARN  rust_team::validate] cause: missing ZULIP_USER and/or ZULIP_TOKEN environment variables
[ERROR rust_team::validate] validation error: Access for the repo rust-lang/wg-debugging is invalid: the team 'rust-lang/wg-debugging' is archived. Please remove it from the repo.
[ERROR rust_team] 1 validation errors found

@marcoieni marcoieni force-pushed the improve-repo-validation-error-message branch 2 times, most recently from 945be99 to a60dd9d Compare April 4, 2025 11:35
@marcoieni marcoieni marked this pull request as ready for review April 4, 2025 11:38
@marcoieni marcoieni mentioned this pull request Apr 4, 2025
Open
Kobzol
Kobzol reviewed Apr 4, 2025
View reviewed changes
src/validate.rs Outdated
@@ -868,20 +868,26 @@ fn validate_repos(data: &Data, errors: &mut Vec<String>) {
}
for team_name in repo.access.teams.keys() {
if !github_teams.contains(&(repo.org.clone(), team_name.clone())) {
let is_team_archived = data.archived_teams().any(|t| t.name() == team_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should start taking organizations into account :) Ideally by creating some form of Team/RepoId, which contains both the org and the name. To avoid naming collisions across different organizations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. Can this be a separate issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but I would rather not add new code that doesn't consider organizations. There's a possibly bigger problem here though, as you should check the github team name (and organization), not the team team name (yeah, it's confusing.. xD).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean I should check the github field?

team/rust_team_data/src/v1.rs

Line 49 in 92c25e8

pub org: String,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, because access.teams refers to github team names, not team team names xD

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 4, 2025

Dry-run check results 

[WARN  sync_team] sync-team is running in dry mode, no changes will be applied.
[INFO  sync_team] synchronizing github

@marcoieni marcoieni force-pushed the improve-repo-validation-error-message branch from a60dd9d to d13f963 Compare April 4, 2025 14:25
@apiraino apiraino mentioned this pull request Apr 4, 2025
Merged
@apiraino
Copy link
Contributor

apiraino commented Apr 4, 2025 •
edited
Loading

Access for the repo rust-lang/wg-debugging is invalid: the team 'rust-lang/wg-debugging' is archived. Please remove it from the repo.

The new version of the validation error is still not very clear to me (that I don't know a lot about the team repository). What should I remove? Which repo? I've deleted this line and the validation now pass but I can't correlate my change to the validation error message.

How about this:

Access for the repo rust-lang/wg-debugging is invalid: the team 'rust-lang/wg-debugging' is archived. Please remove the team access from the [access.teams] section.

@marcoieni marcoieni force-pushed the improve-repo-validation-error-message branch from d13f963 to d482fcb Compare April 7, 2025 14:47
@marcoieni
Copy link
Member Author

rephrased, thanks a lot! This kind of feedback is very useful 🙏

jdno
jdno reviewed Apr 9, 2025
View reviewed changes
src/validate.rs
@@ -868,20 +868,25 @@ fn validate_repos(data: &Data, errors: &mut Vec<String>) {
}
for team_name in repo.access.teams.keys() {
if !github_teams.contains(&(repo.org.clone(), team_name.clone())) {
let error_reason = if data.is_team_archived(team_name, &repo.org) {
"is archived. Please remove the team access from the `[access.teams]` section of the repo"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if something like this wouldn't be even easier to understand. Might require a change to the data model, though, to deserialize a repository without the access.teams field set. 🤔

Suggested change
"is archived. Please remove the team access from the `[access.teams]` section of the repo"
"is archived. Please remove the `[access.teams]` section in the repo's toml"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change sounds more like #1754
Why should we suggest removing the entire [access.teams] section of the repo here?
That particular team is archived, but not the repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdno jdno jdno left review comments

@Kobzol Kobzol Kobzol left review comments

@Mark-Simulacrum Mark-Simulacrum Awaiting requested review from Mark-Simulacrum Mark-Simulacrum is a code owner

@pietroalbini pietroalbini Awaiting requested review from pietroalbini pietroalbini is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcoieni @apiraino @jdno @Kobzol