Remove daily docs branches #3061
Conversation
sima-zhu
requested review from
danieljurek and
weshaggard
and removed request for
a team
|
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: |
danieljurek
left a comment
danieljurek
left a comment
There was a problem hiding this comment.
- This pipeline is currently scheduled to run weekly
- This only generates
BranchPrefixfor the day from a week ago - If the pipeline were modified to run daily this could work (I don't seen any obvious failures in the history of scheduled runs)
I think in this case we'd want to have a script which fetches the names of the branches starting with daily/, determine if a given branch should be deleted, then sends the branch off for deletion using Delete-RemoteBranches.ps1. This would give us multiple attempts at deleting a branch and not couple the deletion process to the pipeline schedule.
@danieljurek Since the daily branches are not necessary to be accurate to one week. How do you think we delete the ones in last month? The problem for this approach is we probably only have one daily branch at the first day of the month. If this causes us pain, we can add steps to return a range of branches then loop into each one. |
sima-zhu
added
the
Central-EngSys
sima-zhu
changed the title
eng/pipelines/branch-cleanup.yml
Outdated
| - azure-sdk-for-net | ||
| - azure-sdk-for-python | ||
|
|
||
| - name: AzureDocsRepo |
There was a problem hiding this comment.
Why add another set of parameters? If the only thing different is a branch pattern we should just add that as a property of the repo so we can use the same logic for everything.
There was a problem hiding this comment.
|
@sima-zhu -- Date math is a very intricate domain. Libraries are almost always the right way to go. If we're reusing the Get a list of all existing branches that start with Do some parsing: $parsedDates = $dates `
| ForEach-Object { ($_ -split '/')[1] } ` # <-- TODO: handle removing the potential '[-ci-(succeeded|failed)]' scenario
| ForEach-Object {
$outpuut = New-Object datetime;
$ignore = [datetime]::TryParseExact(
$_,
'yyyy-MM-dd',
[System.Globalization.CultureInfo]::InvariantCulture,
[System.Globalization.DateTimeStyles]::None,
[ref]$a
);
return $output } Compare the dates: $prefixesToDelete = $parsedDates `
| Where-Object { ((Get-Date - $_).TotalDays -gt 7 } ` # <-- Date math happens here
| ForEach-Object { "daily/$($_.ToString('yyyy-MM-dd'))" } Delete the branches: foreach ($prefix in $prefixesToDelete) {
$(System.DefaultWorkingDirectory)/eng/common/scripts/Delete-RemoteBranches.ps1
-RepoOwner "MicrosoftDocs"
-RepoName ${{ repo }}
-BranchPrefix $prefix # <-- Delete prefix from the list
-AuthToken $(azuresdk-github-pat)
} |
|
Just for my understanding are we trying to delete all the daily branches that are older than a week old? If so, should we be using the branch name or the date of the last commit into a branch? If we do the later that might be a more general approach for all the branch clean-up. |
I do worry here if there are scenarios where you cut a new branch but don't rev any commits, resulting in newer branches being deleted when they shouldn't. Also, we would need a pretty explicit branch pattern to make sure we don't accidentally pick up other random branches, and if we're already checking an explicit branch pattern we might as well just encode the entire date in it and delete it via @danieljurek's scripts? |
|
Our current automation for creating daily branches don't create new branches unless the automation is also pushing commits to those branches... If someone were to manually create a branch which met whatever filtration criteria we set then it's possible that such a script would delete the branch out from under them... though they could encounter similar problems if the branch they created had a name that met the criteria that we're explicitly parsing here as well. When configuring automation based on branch name and most recent commit date, one would want to avoid matching on things like Also, commit date can, with some work, be modified by the user but I think that's pretty far out of the scope of what we care about. I think we have other branch cleanup scripts for things like |
|
@danieljurek @benbp I am thinking adding another parameters like branch creation date range, so we can solve the problem without looping on top of the script step. |
sima-zhu
changed the title
|
The following pipelines have been queued for testing: |
|
The following pipelines have been queued for testing: |
|
The following pipelines have been queued for testing: |
| $RepoName = ($Repo -split "/")[1] | ||
| $GithubAPIBaseURI = "repos/$RepoOwner/$RepoName" | ||
|
|
||
| function HandleGHAPICall ($api, $method="GET") { |
There was a problem hiding this comment.
Switch to gh, as the github API has no ability to call the commit sha url directly which introduce more complexity to the script.
There was a problem hiding this comment.
We can use Invoke-RestMethod, but make the script combined with Github API helper method and rest api call. GH is flexible in this case.
There was a problem hiding this comment.
If we are switching to using gh commands directly why do we need to use the raw api calls as opposed to the higher level gh commands? This approach seems as though we are just replacing Invoke-RestMethod with gh api and I'm not sure why that is helpful.
|
The following pipelines have been queued for testing: |
|
The following pipelines have been queued for testing: |
|
The following pipelines have been queued for testing: |
|
The following pipelines have been queued for testing: |
|
The following pipelines have been queued for testing: |
| } | ||
| catch { | ||
| LogError "Get-GithubReferenceCommitDate failed with exception:`n$_" | ||
| exit 1 |
There was a problem hiding this comment.
We should watch to ensure this does not happen in any normal cases otherwise we might get to a spot where this clean-up continuously fail.
weshaggard
left a comment
weshaggard
left a comment
There was a problem hiding this comment.
Finish testing but I think the changes are ready now.
sima-zhu
force-pushed
the
clean_docs_daily
branch
from
c6103ac to
be21b49
Compare
|
The following pipelines have been queued for testing: |
|
Hello @azure-sdk! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
6 participants
https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1533820&view=results