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

Add repo-specific condition to labeling workflows #112169

Merged
merged 6 commits into from
Feb 6, 2025

Conversation

carlossanlop
Copy link
Member

Follow-up of #112161

Condition the labeling workflows so they only work in dotnet/runtime. Also improve readme as suggested, and also add @jeffhandley as explicit codeowner for workflows.

@carlossanlop carlossanlop self-assigned this Feb 5, 2025
@carlossanlop carlossanlop requested a review from a team as a code owner February 5, 2025 01:16
Copy link
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@danmoseley
Copy link
Member

Are you going to put this into arcade, and if so will other repos extend this if? Is there some way to pass the setting externally to achieve the same thing but without making this less reusable? Nit only

.github/workflows/README.md Outdated Show resolved Hide resolved
@@ -12,6 +12,7 @@ on:

jobs:
check-labels:
if: github.repository == 'dotnet/runtime'
Copy link
Member

Choose a reason for hiding this comment

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

To @danmoseley's question:

Are you going to put this into arcade, and if so will other repos extend this if? Is there some way to pass the setting externally to achieve the same thing but without making this less reusable? Nit only

I spent some time searching across GitHub docs and code searches, a grep.app search, and asking Copilot for best practices around this, and hard-coding either the repository name or just the repository owner name are indeed the state of the art here. Theoretically, we could set up an environment variable that would be available to all our workflow runners, and we could check that environment variable, but since I don't see anyone else doing that I think the consensus is that making this more dynamic isn't worth the effort.

For any workflow that is going to be shared across repos (or at least referenced as something that could be copied into other repos), we'd want to just check the repository_owner like we do in locker.yml. For these workflows, either approach should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

.github/workflows/README.md Show resolved Hide resolved
@carlossanlop
Copy link
Member Author

@jeffhandley I applied your suggestions, thanks. If there's anything else to address, I can happily do it post-merge. I am seeing a bunch of "Approve workflow" buttons in servicing, so I would like to merge this PR to confirm that merging only for main will take care of servicing too.

@carlossanlop carlossanlop merged commit 9caf9f1 into dotnet:main Feb 6, 2025
16 checks passed
@carlossanlop carlossanlop deleted the FixWorkflows2 branch February 6, 2025 00:58
@jeffhandley
Copy link
Member

Looks good, @carlossanlop; thanks.

For pull requests into the servicing branches, if those pull requests contain workflows that use pull_request events, and that workflow exists in the target branch, I expect you'll see the "Approve workflow" buttons. We will essentially have to flush out all forks/branches based on the pull_request event for that button to go away. The options are to approve the workflows (after confirming the workflows are unmodified in the pull request), or to update the pull request to get the latest commits from the target branch so that the workflow is based on pull_request_target.

@carlossanlop
Copy link
Member Author

@jeffhandley, right, but that sounds like we still need to backport the change to release/X.0 and release/X.0-staging branches, to ensure that when they act as the target branch for a servicing PR, the pull_request_target is used. Am I correct?

grendello added a commit to grendello/runtime that referenced this pull request Feb 6, 2025
* main: (23 commits)
  add important remarks to NrbfDecoder (dotnet#111286)
  docs: fix spelling grammar and missing words in clr-code-guide.md (dotnet#112222)
  Consider type declaration order in MethodImpls (dotnet#111998)
  Add a feature flag to not use GVM in Linq Select (dotnet#109978)
  [cDAC] Implement ISOSDacInterface::GetMethodDescPtrFromIp (dotnet#110755)
  Restructure JSImport/JSExport generators to share more code and utilize more Microsoft.Interop.SourceGeneration shared code (dotnet#107769)
  Add more detailed explanations to control-flow RegexOpcode values (dotnet#112170)
  Add repo-specific condition to labeling workflows (dotnet#112169)
  Fix bad assembly when a nested exported type is marked via link.xml (dotnet#107945)
  Make `CalculateAssemblyAction` virtual. (dotnet#112154)
  JIT: Enable reusing profile-aware DFS trees between phases (dotnet#112198)
  Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory (dotnet#111877)
  JIT: Support custom `ClassLayout` instances with GC pointers in them (dotnet#112064)
  Factor positive lookaheads better into find optimizations (dotnet#112107)
  Add ImmutableCollectionsMarshal.AsMemory (dotnet#112177)
  [mono] ILStrip write directly to the output filestream (dotnet#112142)
  Allow the NativeAOT runtime pack to be specified as the ILC runtime package (dotnet#111876)
  JIT: some reworking for conditional escape analysis (dotnet#112194)
  Replace HELPER_METHOD_FRAME with DynamicHelperFrame in patchpoints (dotnet#112025)
  [Android] Decouple runtime initialization and entry point execution for Android sample (dotnet#111742)
  ...
@jeffhandley
Copy link
Member

Oh gosh; you're right, @carlossanlop. It wasn't clicking for me that the release branches are the "target" here. Thanks for clarifying.

carlossanlop added a commit to carlossanlop/runtime that referenced this pull request Feb 6, 2025
* Condition labeling workflows to only run on dotnet/runtime.
* Improve readme
* Add jeffhandley as explicit workflow owner

Co-authored-by: Jeff Handley <[email protected]>
carlossanlop added a commit to carlossanlop/runtime that referenced this pull request Feb 6, 2025
* Condition labeling workflows to only run on dotnet/runtime.
* Improve readme
* Add jeffhandley as explicit workflow owner

Co-authored-by: Jeff Handley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants