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

Add several syntactical, functional and logic improvements (#11) #595

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
+42 −58
Open

Add several syntactical, functional and logic improvements (#11) #595

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
91e27bd
Add several syntactical, functional and logical improvements (#11)
aleks-ivanov May 20, 2021
b218db8
Merge branch 'vnext' into feature/improve-ci-cd
aleks-ivanov May 31, 2021
1004e99
Merge branch 'microsoft:vnext' into feature/improve-ci-cd
aleks-ivanov Jun 21, 2021
97b2a50
Replace release action with GH CLI (#17)
aleks-ivanov Jun 21, 2021
7f9e6b8
switch release branch to master (#19)
aleks-ivanov Jun 28, 2021
22c0ee4
Merge branch 'vnext' into feature/improve-ci-cd
aleks-ivanov Apr 24, 2022
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
67 changes: 38 additions & 29 deletions .github/workflows/ci-cd.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
name: CI/CD Pipeline

on: [push, pull_request, workflow_dispatch]
on: [ push, pull_request, workflow_dispatch ]
darrelmiller marked this conversation as resolved.
Show resolved Hide resolved

jobs:
ci:
name: Continuous Integration
runs-on: ubuntu-latest
outputs:
latest_version: ${{ steps.tag_generator.outputs.new_version }}
is_default_branch: ${{ steps.conditionals_handler.outputs.is_default_branch }}
darrelmiller marked this conversation as resolved.
Show resolved Hide resolved
env:
ARTIFACTS_FOLDER: ${{ github.workspace }}/Artifacts
GITHUB_RUN_NUMBER: ${{ github.run_number }}
Expand All @@ -23,7 +22,7 @@ jobs:
shell: pwsh
run: |
# Get default branch
$repo = 'microsoft/OpenAPI.NET'
$repo = "${{ github.repository }}"
darrelmiller marked this conversation as resolved.
Show resolved Hide resolved
$defaultBranch = Invoke-RestMethod -Method GET -Uri https://api.github.com/repos/$repo | Select-Object -ExpandProperty default_branch
Write-Output "::set-output name=default_branch::$(echo $defaultBranch)"

Expand All @@ -33,11 +32,22 @@ jobs:
run: |
$defaultBranch = "${{ steps.data_gatherer.outputs.default_branch }}"
$githubRef = "${{ github.ref }}"
$githubEventName = "${{ github.event_name }}"
$isDefaultBranch = 'false'
$isPush = 'false'
$isPushToDefaultBranch = 'false'
if ( $githubRef -like "*$defaultBranch*" ) {
$isDefaultBranch = 'true'
}
if ( $githubEventName -eq 'push' ) {
$isPush = 'true'
}
if ( $githubRef -like "*$defaultBranch*" -and $githubEventName -eq 'push' ) {
$isPushToDefaultBranch = 'true'
}
Write-Output "::set-output name=is_default_branch::$(echo $isDefaultBranch)"
Write-Output "::set-output name=is_push::$(echo $isPush)"
Write-Output "::set-output name=is_push_to_default_branch::$(echo $isPushToDefaultBranch)"
darrelmiller marked this conversation as resolved.
Show resolved Hide resolved

- name: Checkout repository
id: checkout_repo
Expand All @@ -46,7 +56,7 @@ jobs:
token: ${{ secrets.GITHUB_TOKEN }}
fetch-depth: 0

- if: steps.conditionals_handler.outputs.is_default_branch == 'true'
- if: steps.conditionals_handler.outputs.is_push_to_default_branch == 'true'
Copy link
Contributor Author

@aleks-ivanov aleks-ivanov May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Used the newly added is_push_to_default_branch condition, instead of the is_default_branch, to further restrict the new tag generating step to only execute when a push commit to the default branch is made.

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 somewhat confused by this process. First, the default branch is vnext, not master. We don't create releases when we are updating vnext. Also, we don't push directly to vnext, we generally PR to it. What am I missing?

Copy link
Contributor Author

@aleks-ivanov aleks-ivanov May 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We don't create releases when we are updating vnext

If you are not making releases from your default branch (vnext), we will change it to the correct one. Is it master or another branch ?

Also not every push commit creates a new GitHub tag and release. In the documentation we submitted with the last PR, you can check out the exact prerequisite to trigger an output (meaning create a new tag) from the "Bump GH tag" step, which in turn triggers a GitHub release.

The path is this:

Push commit in release branch with SemVer label in the commit message
-> "Bump GH tag" step creates a new GitHub tag and pushes it to the repo
-> CD job evaluates if there is a new GitHub tag and if there is, executes
-> the "Create and publish release" step in the CD job uses the newly created and pushed GitHub tag to create a GitHub release

we generally PR to it

The merge of a PR triggers a push commit, so you can either create a release though your release branch, by committing into it or merging a PR.

If you have any other questions about the automated release process, please let us know! 🙂

Copy link
Member

Choose a reason for hiding this comment

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

We create releases off the master branch.

Copy link
Contributor Author

@aleks-ivanov aleks-ivanov Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@darrelmiller with the last commit:

  • switch the release branch from default_branch to master
  • remove the data gatherer and conditionals handler steps, since they were used specifically for the default_branch as release branch logic

In the current state of the pipeline if there are no errors present, a push commit or PR merge to the master branch with a SemVer label in the title, would deploy (publish packages and create a GitHub release).

Push commits or PR merges to the default_branch (or any other branch, except the default_branch) would trigger the pipeline, but not deploy.

name: Bump GH tag
id: tag_generator
uses: mathieudutour/github-tag-action@v5.4
Expand All @@ -59,41 +69,40 @@ jobs:
id: build_projects
shell: pwsh
run: |
$projectsArray = @(
'.\src\Microsoft.OpenApi\Microsoft.OpenApi.csproj',
'.\src\Microsoft.OpenApi.Readers\Microsoft.OpenApi.Readers.csproj',
'.\src\Microsoft.OpenApi.Tool\Microsoft.OpenApi.Tool.csproj'
)
$gitNewVersion = if ("${{ steps.tag_generator.outputs.new_version }}") {"${{ steps.tag_generator.outputs.new_version }}"} else {$null}
$projectCurrentVersion = ([xml](Get-Content .\src\Microsoft.OpenApi\Microsoft.OpenApi.csproj)).Project.PropertyGroup.Version
$projectCurrentVersion = ([xml](Get-Content -Path .\src\Microsoft.OpenApi\Microsoft.OpenApi.csproj)).Project.PropertyGroup.Version
darrelmiller marked this conversation as resolved.
Show resolved Hide resolved
$projectNewVersion = $gitNewVersion ?? $projectCurrentVersion

$projectsArray | ForEach-Object {
dotnet build $PSItem `
-c Release # `
# -o $env:ARTIFACTS_FOLDER `
# /p:Version=$projectNewVersion
Get-ChildItem -Path src/ -Filter *.csproj -Exclude *Workbench* -File -Recurse | ForEach-Object {
Copy link
Contributor Author

@aleks-ivanov aleks-ivanov May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Replaced static array of project locations with a more elegant solution.

Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, any reason why you don't simply target the sln?

Copy link
Contributor Author

@aleks-ivanov aleks-ivanov May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We just took the build.cmd file as an example and just never gave it a second thought, but now that you mention it unless there is a specific reason not to, we could change the build step to building only the solution instead 🙂

Copy link
Member

Choose a reason for hiding this comment

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

We don't publish the Workbench tool, so I didn't bother building it for publishing nugets. I don't have a problem building it as part of the pipeline. However currently dotnet build can't build the Workbench project because of some error about the GenerateResource task. Not sure what that is about.

dotnet build $PSItem.FullName `
--configuration Release # `
# --output $env:ARTIFACTS_FOLDER `
# -property:Version=$projectNewVersion
}

# Move NuGet packages to separate folder for pipeline convenience
# New-Item Artifacts/NuGet -ItemType Directory
# Get-ChildItem Artifacts/*.nupkg | Move-Item -Destination "Artifacts/NuGet"
# New-Item -Name Artifacts/NuGet -ItemType Directory
# Get-ChildItem -Path Artifacts/ -Filter *.nupkg | Move-Item -Destination Artifacts/NuGet
darrelmiller marked this conversation as resolved.
Show resolved Hide resolved

- name: Run unit tests
id: run_unit_tests
shell: pwsh
run: |
$testProjectsArray = @(
'.\test\Microsoft.OpenApi.Tests\Microsoft.OpenApi.Tests.csproj',
'.\test\Microsoft.OpenApi.Readers.Tests\Microsoft.OpenApi.Readers.Tests.csproj',
'.\test\Microsoft.OpenApi.SmokeTests\Microsoft.OpenApi.SmokeTests.csproj'
)

$testProjectsArray | ForEach-Object {
dotnet test $PSItem `
-c Release
Get-ChildItem -Path test/ -Filter *.csproj -File -Recurse | ForEach-Object {
$fileBaseName = $PSItem.Basename
dotnet test $PSItem.FullName `
--configuration Release `
--logger "trx;LogFileName=$fileBaseName.trx;verbosity=normal" `
--results-directory TestResults/
darrelmiller marked this conversation as resolved.
Show resolved Hide resolved
}

- name: Upload test results as artifacts
id: ul_testresults_artifact
uses: actions/upload-artifact@v1
with:
name: TestResults
path: TestResults/

darrelmiller marked this conversation as resolved.
Show resolved Hide resolved
# - if: steps.tag_generator.outputs.new_version != ''
# name: Upload NuGet packages as artifacts
# id: ul_packages_artifact
Expand All @@ -103,7 +112,7 @@ jobs:
# path: Artifacts/NuGet/

cd:
if: needs.ci.outputs.is_default_branch == 'true' && needs.ci.outputs.latest_version != ''
if: needs.ci.outputs.latest_version != ''
darrelmiller marked this conversation as resolved.
Show resolved Hide resolved
name: Continuous Deployment
needs: ci
runs-on: ubuntu-latest
Expand All @@ -120,8 +129,8 @@ jobs:
# continue-on-error: true
# shell: pwsh
# run: |
# Get-ChildItem NuGet/*.nupkg | ForEach-Object {
# nuget push $PSItem `
# Get-ChildItem -Path NuGet/ -Filter *.nupkg | ForEach-Object {
darrelmiller marked this conversation as resolved.
Show resolved Hide resolved
# nuget push $PSItem.FullName `
darrelmiller marked this conversation as resolved.
Show resolved Hide resolved
# -ApiKey $env:NUGET_API_KEY `
# -Source https://api.nuget.org/v3/index.json
# }
Expand Down