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

Update SDK-PR-guide.md to include branch lockdown label #46337

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

marcpopMSFT
Copy link
Member

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Jan 27, 2025
@marcpopMSFT marcpopMSFT requested a review from baronfel January 27, 2025 20:54
if [[ "$current_date" > "$third_tuesday" || "$current_date" < "$first_tuesday" ]]; then
echo "Within the label period. Adding labels to PRs..."

# Get all open PRs created by app/dotnet-maestro and targeting branches release/8* and release/9* excluding release/9.0.3xx
Copy link
Member

Choose a reason for hiding this comment

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

So, our branch-to-branch codeflow PRs use app/github-actions. Also, why are we limiting the PRs based on who created them? If the branch is locked down, all PRs targeting those branches should have "Branch Lockdown", right?

Copy link
Member Author

Choose a reason for hiding this comment

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

At least today, I use branch lockdown also when the branch unlocks to find all PRs that should go in. If I don't limit this, I'll pick up other PRs that just happened to target those branches. Not a good excuse and I think your argument to just tag all of them is probably stronger.

.github/workflows/add-lockdown-label.yml Show resolved Hide resolved
Comment on lines +20 to +21
- name: Install jq
run: sudo apt-get install -y jq
Copy link
Member

Choose a reason for hiding this comment

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

What it jq? It looks to be some kind of filtering mechanism (like what is native to PowerShell) but as a separate Bash module.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a json parsing tool that copilot recommended.

Copy link
Member

Choose a reason for hiding this comment

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

jq is very commonly used in GitHub workflows. Great command-line tool for processing JSON data.

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 not trying to promote it, but PowerShell can natively parse JSON. Apparently, you can just set the shell for use in these steps.

Comment on lines 5 to 6
# Run daily at 00:00 UTC
- cron: '0 23 * * *'
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 not convinced this is the right approach overall. The reason is, this runs once a day. Standard label bots run as PRs are created. That's what triggers them to run. Sure, this process will work on pre-existing PRs just fine. But as things attempt to "flow" from other repos during the day, domestic cat might accidentally merge things they shouldn't.

Also, triggering this at the "start of the day" like this, will there be any issues with the date calculation below? Most processes like this avoid the exact start of a day because stuff like daylight savings time exists. Plus, this runs right in the middle of Jason's shift, I believe.

Copy link
Member Author

Choose a reason for hiding this comment

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

So you think it should trigger on every PR do these branches then? That means it runs a lot of times but maybe that's ok. I wonder then if it runs on PRs, if we could just run it against the PR that's being created rather than querying all the PRs.

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 fairly certain you can trigger on PR creation. But I don't know GitHub Actions.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you can. I believe you can also trigger it on other PR events (opened, closed, assigned, etc...)
https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request

@marcpopMSFT
Copy link
Member Author

/backport to release/9.0.2xx

Copy link
Contributor

Started backporting to release/9.0.2xx: https://github.com/dotnet/sdk/actions/runs/13079866389

The prior version of this would only label new PRs not PRs created before the date. This searches all PRs that match the requirements whenever a new PR is created targeting these branches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants