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

Add an option to only announce PRs from specific users #26

Merged
merged 8 commits into from
Jun 6, 2024
Merged

Add an option to only announce PRs from specific users #26

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aebf6f6
add included users list
aracho1 May 23, 2024
e0f019f
rename the list
aracho1 May 30, 2024
a21da03
update variable name
aracho1 May 30, 2024
bf4398d
update read me
aracho1 May 31, 2024
b2c2a05
rename list again
aracho1 May 31, 2024
629ce54
update readme
aracho1 May 31, 2024
df509d3
refactor and change announced_users type
aracho1 Jun 4, 2024
f05d272
check for empty value
aracho1 Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ This action automatically sends a message to google chats detailing the list of

**Required** PR labels to ignore when scanning for PR's. Defaults to `Stale`

## `github-announced-users`

**Optional** Only Github users to announce PR's from. If set, other users' PR's will be ignored.

## Example usage

```yaml
Expand All @@ -48,6 +52,9 @@ export GOOGLE_WEBHOOK_URL=https://chats.google.com...
export GITHUB_IGNORED_USERS=49699333
# List of labels to ignore when scanning for PR's
export GITHUB_IGNORED_LABELS=dependencies
# List of users to announce PR's from (if set, other users will be ignored)
# (e.g. 49699333 is dependabot, 19733683 is snykbot, 108136057 is scala steward)
export GITHUB_ANNOUNCED_USERS=49699333,19733683,108136057

cargo run
```
Expand Down
5 changes: 5 additions & 0 deletions action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ inputs:
description: "Comma delimited list of user ID's to ignore"
required: true
default: "49699333"
github-announced-users:
description: "Comma delimited list of user ID's to announce"
required: false
default: ""
github-ignored-labels:
description: "Comma delimited list of label names to ignore"
required: true
Expand All @@ -32,6 +36,7 @@ runs:
GITHUB_REPOSITORIES: ${{ inputs.github-repositories }}
GITHUB_TOKEN: ${{ inputs.github-token }}
GITHUB_IGNORED_USERS: ${{ inputs.github-ignored-users }}
GITHUB_ANNOUNCED_USERS: ${{ inputs.github-announced-users }}
Copy link
Member

Choose a reason for hiding this comment

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

Not quite sure how Github handles this if inputs.github-announced-users isn't defined, it might interpet it as an empty string:

GITHUB_ANNOUNCED_USERS: ""

In which case our code to check if the environment variable is present or not won't correctly detect that the GITHUB_ANNOUNCED_USERS is undefined. Sorry to suggest Option but now I'm thinking just checking the length of our announced users array might be better than checking if the variable is present or not!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good spot, thanks @AshCorr. How about we do something like this to handle the empty string?

let announced_users: Option<Vec<i32>> = env::var("GITHUB_ANNOUNCED_USERS")
        .ok()
        .and_then(|s| {
            if s.is_empty() {
                None
            } else {
                Some(s.split(',').map(|id| id.parse().unwrap()).collect())
            }
        });

Copy link
Member

Choose a reason for hiding this comment

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

Aha! Yea that would work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed the change, happy to +1 @AshCorr ?

GITHUB_IGNORED_LABELS: ${{ inputs.github-ignored-labels }}
GOOGLE_WEBHOOK_URL: ${{ inputs.google-webhook-url }}
SHOW_PR_AGE: ${{ inputs.show-pr-age }}
16 changes: 16 additions & 0 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ async fn scan_repository(
repository_name: String,
github_token: &String,
ignored_users: &Vec<&str>,
announced_users: &Vec<&str>,
ignored_labels: &Vec<&str>,
) -> Result<Vec<GithubPullRequest>, Error> {
info!("Starting PR scan of {}", repository_name);
Expand Down Expand Up @@ -51,6 +52,18 @@ async fn scan_repository(
continue;
}

if !announced_users.contains(&pull_request.user().id().to_string().as_str()) {
Copy link
Member

@AshCorr AshCorr May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think announced_users will default to an empty vector? Would this mean that the default would be to ignore everyone?

Wondering if we need to make announced_users a Optional<Vec<&str>> instead and check if it exists here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, thank you for the suggestion!

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively check if the Vec is empty and skip this check if it is, I prefer the explicit Optional however.

Copy link
Contributor

@marsavar marsavar May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with Ash here, with a preference for the option type.

Also, since you know user id's are of type i32, you could parse them here already so you avoid the awkward conversion &pull_request.user().id().to_string().as_str().

I.e.
You could do something like

    let announced_users: Option<Vec<i32>> = env::var("GITHUB_ANNOUNCED_USERS")
        .ok()
        .map(|s| s.split(',').map(|id| id.parse().unwrap()).collect());

And then change the signature of scan_repository so that it accepts announced_users: Option<&[i32]>

so the final check would look something like

        if let Some(announced_users) = announced_users {
            if !announced_users.contains(pull_request.user().id()) {
                info!("Users to announce: {:?}", announced_users);
                info!(
                "Ignoring PR {}({}) as it was raised by a user not included in the announced users list {}({})",
                pull_request.id(),
                pull_request.title(),
                pull_request.user().id(),
                pull_request.user().login()
            );
                continue;
            }
        }

info!("Users to announce: {:?}", announced_users);
info!(
"Ignoring PR {}({}) as it was raised by a user not included in the announced users list {}({})",
pull_request.id(),
pull_request.title(),
pull_request.user().id(),
pull_request.user().login()
);
continue;
}

let mut has_ignore_label = false;

for label in pull_request.labels() {
Expand Down Expand Up @@ -113,6 +126,8 @@ async fn main() -> Result<(), Error> {
env::var("GOOGLE_WEBHOOK_URL").context("GOOGLE_WEBHOOK_URL must be set")?;
let ignored_users: String = env::var("GITHUB_IGNORED_USERS").unwrap_or("".to_string());
let ignored_users: Vec<&str> = ignored_users.split(",").collect();
let announced_users: String = env::var("GITHUB_ANNOUNCED_USERS").unwrap_or("".to_string());
let announced_users: Vec<&str> = announced_users.split(",").collect();
let ignored_labels: String = env::var("GITHUB_IGNORED_LABELS").unwrap_or("".to_string());
let ignored_labels: Vec<&str> = ignored_labels.split(",").collect();
let show_pr_age: bool = env::var("SHOW_PR_AGE")
Expand All @@ -127,6 +142,7 @@ async fn main() -> Result<(), Error> {
repository.to_string(),
&github_token,
&ignored_users,
&announced_users,
&ignored_labels,
)
.await?,
Expand Down
Loading