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

Beejones/initial release management #299

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

Conversation

beejones
Copy link
Contributor

Add Release Management to KMS

We add a GitHub workflow which calls CI to do a full test first. Next, push assets to the referenced tag including:

  • The KMS app
  • Release notes
  • SBOM

uses: ./.github/workflows/system-tests.yml
secrets:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason we changed this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

permissions:
contents: write
actions: write
checks: write
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure we need all these checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am planning to test this.

tests:
name: Tests
uses: ./.github/workflows/ci.yml
secrets:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just have secrets: inherit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried that first and it

run: |
echo "## Release Notes" > RELEASE_NOTES.md
echo "" >> RELEASE_NOTES.md
PRS=$(curl -s -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another way we could do this is enforce squash and merge on PRs and then inspect the commit history

This will catch changes done directly to main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

upload_url: ${{ steps.create_release.outputs.upload_url }}
asset_path: ./RELEASE_NOTES.md
asset_name: RELEASE_NOTES.md
asset_content_type: text/markdown
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also include constitution files? Names actions/kms.js

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe even a whole KMS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is all in the sources in zip file

@@ -13,9 +13,13 @@ on:
inputs:
test:
type: string
secrets:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this is needed?

@@ -36,22 +40,18 @@ jobs:
- name: Checkout
uses: actions/checkout@v4

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you have some merge conflicts here

@@ -7,6 +7,11 @@ permissions:
on:
workflow_dispatch:
workflow_call:
secrets:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this?

@@ -27,7 +32,9 @@ jobs:
test:
name: ${{ '' }}
needs: discover-tests
secrets: inherit # pragma: allowlist secret
secrets:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not secrets: inherit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -40,14 +44,10 @@ jobs:
env:
GH_TOKEN: ${{ github.token }}
run: ./scripts/tools/install-deps.sh


- name: Log into Azure
uses: azure/login@v2
with:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You've deleted a bunch of comments that you asked me to add originally, have you changed your mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

accidental delete. Thanks for catching

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants