-
Notifications
You must be signed in to change notification settings - Fork 4
Add workflows to release, version, and list Connect extensions #35
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
Changes from all commits
61bea63
fa01edd
ad87004
0cd1ad1
1e7675b
f2969fc
c1bbb40
dab3a99
e534088
b3c6401
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| name: Lint Extension | ||
| description: Lint an extension for release | ||
|
|
||
| inputs: | ||
| extension-name: | ||
| description: The name of the extension | ||
| required: true | ||
| type: string | ||
|
|
||
| runs: | ||
| using: "composite" | ||
|
|
||
| steps: | ||
| # Ensures that the manifest.json for the given extension name | ||
| # contains all the required fields for the rest of the release workflow | ||
| - run: | | ||
| jq ' | ||
| if (.extension | type) != "object" then error("Missing extension object") | ||
| elif (.extension.name | type) != "string" then error("Missing extension.name") | ||
| elif (.extension.title | type) != "string" then error("Missing extension.title") | ||
| elif (.extension.description | type) != "string" then error("Missing extension.description") | ||
| elif (.extension.homepage | type) != "string" then error("Missing extension.homepage") | ||
| elif (.extension.version | type) != "string" then error("Missing extension.version") | ||
| else . end | ||
| ' ./extensions/${{ inputs.extension-name }}/manifest.json | ||
|
Comment on lines
+17
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a lot of inline
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh that is a good idea. I can look into that. It would be helpful to do a pre-push lint.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to look at this as a follow-up in combination with #35 (comment) |
||
| shell: bash | ||
|
|
||
| - uses: actions/setup-node@v4 | ||
|
|
||
| - run: npm install -g semver | ||
| shell: bash | ||
|
|
||
| # The semver must be valid for the sorting, comparisons, and release | ||
| # process to work | ||
| - name: Check for valid semver | ||
| run: | | ||
| semver -c $(jq -c -r '.extension.version' < ./extensions/${{ inputs.extension-name }}/manifest.json) | ||
| shell: bash | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,45 @@ | ||||
| name: Package Extension | ||||
| description: Package an extension into a tarball for release | ||||
|
|
||||
| inputs: | ||||
| extension-name: | ||||
| description: The name of the extension | ||||
| required: true | ||||
| type: string | ||||
| artifact-name: | ||||
| description: The name of the artifact | ||||
| required: false | ||||
| type: string | ||||
|
|
||||
| runs: | ||||
| using: "composite" | ||||
|
|
||||
| steps: | ||||
| # If we are not passed an artifact to use as the source for the tarball | ||||
| # we will create a tarball from the extension's directory | ||||
|
Comment on lines
+18
to
+19
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly for my own education: it's not immediately clear to me when we would take this path versus the other path down below for these. Could you educate me?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely. Currently the only time we do this is in the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The creation and passing of the artifact to this action allows custom workflows to select which files to package.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, so if it's a custom workflow style task, this allows us to build the artifact elsewhere (in the custom workflow) and then pass it to this workflow but/and therefore bypasses the building step in this workflow. Is that a good summary?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, even an accurate summary — doesn't need to be good 😂
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Very close. The only correction I'll make is that this is a composite action. It allows us to build the artifact we want packaged and pass it to this action. It avoids building as part of the packaging step (for those extensions with custom workflows), and simplifies the things that need to be passed to this action.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a requested change, just a question: would it be beneficial to add additional detail such as "this is what we expect during for simple content", and in the next one "we expect artifacts to be passed and processed for complex content". Or is that getting too deep?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept the explanations in the actions a bit lighter since "simple content" probably doesn't need to interact with them. We can definitely add some deeper explanations if we get questions often. |
||||
| - name: Create tar | ||||
| if: ${{ inputs.artifact-name == '' }} | ||||
| run: tar -czf ${{ inputs.extension-name}}.tar.gz ./extensions/${{ inputs.extension-name}} | ||||
| shell: bash | ||||
|
|
||||
| # If we are passed an artifact to use as the source for the tarball | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I love all of the code comments. Keep doing that. 👍 |
||||
| # we will download the artifact and create a tarball from it | ||||
| # this avoids including extra files in the extension | ||||
| - name: Download optional artifact | ||||
| if: ${{ inputs.artifact-name != '' }} | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we offer a message to the user if an error occurs during this process? Or is that even possible?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There will be a failure if the artifact doesn't exist and the logs will give the name and that no artifact matched it. Thats all built in to the download-artifact action. Does that satisfy that you think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I think that makes sense. |
||||
| uses: actions/download-artifact@v4 | ||||
| with: | ||||
| name: ${{ inputs.artifact-name }} | ||||
| path: ${{ inputs.extension-name }} | ||||
|
|
||||
| - name: Create tar from artifact | ||||
| if: ${{ inputs.artifact-name != '' }} | ||||
| run: tar -czf ${{ inputs.extension-name }}.tar.gz ${{ inputs.extension-name }} | ||||
| shell: bash | ||||
|
|
||||
| # Upload the extension's tarball for use in other actions in the workflow | ||||
| - name: Upload extension tar | ||||
| uses: actions/upload-artifact@v4 | ||||
| with: | ||||
| name: ${{ inputs.extension-name }}.tar.gz | ||||
| path: ${{ inputs.extension-name }}.tar.gz | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| name: Release Extension | ||
| description: Release an extension as a GitHub Release | ||
|
|
||
| inputs: | ||
| extension-name: | ||
| description: The name of the extension | ||
| required: true | ||
| type: string | ||
|
|
||
| runs: | ||
| using: "composite" | ||
|
|
||
| steps: | ||
| - uses: actions/setup-node@v4 | ||
|
|
||
| - run: npm install -g semver | ||
| shell: bash | ||
|
|
||
| - name: Get manifest extension version | ||
| run: | | ||
| MANIFEST_VERSION=$(semver -c $(jq -c -r '.extension.version' < ./extensions/${{ inputs.extension-name }}/manifest.json)) | ||
| echo "MANIFEST_VERSION=$MANIFEST_VERSION" >> "$GITHUB_ENV" | ||
| shell: bash | ||
|
|
||
| # Grabs the latest version from the extensions.json file | ||
| # If an extension hasn't been released yet we default to `0.0.0` so any | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want to allow unreleased extensions to be merged to
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't want to:
This may not be the right choice though.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I very much see where you're coming from @cgraham-rs , that theoretically keeping unrelease out of main makes things nice and clean, but PRs into other branches and having long running branches with development versions is advanced enough git that I would like to avoid requiring it if we can. It would be nice to have this noted prominently in a README (if we don't already) that just cause something is on
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Absolutely. The CONTRIBUTING doc we write for adding extensions will have this callout and tips for reading the GitHub Action logs to determine if a PR will result in a release or not 👍 |
||
| # version in the manifest will be higher | ||
| - name: Get lastest version from extension list | ||
| continue-on-error: true | ||
| run: | | ||
| LATEST_VERSION=$(jq -c '.extensions[] | select(.name=="${{ inputs.extension-name }}").latestVersion.version' < extensions.json) | ||
| LATEST_VERSION=$(semver -c "${LATEST_VERSION:-0.0.0}") | ||
| echo "LATEST_VERSION=$LATEST_VERSION" >> "$GITHUB_ENV" | ||
| shell: bash | ||
|
|
||
| # We only want to release if the manifest.json contains a newer semver | ||
| # version than the latest version in the extensions.json | ||
| # We compare that here, and echo if a release will occur | ||
| # This can be helpful when looking at Pull Request action outputs | ||
| # so it is clear what will happen on a merge to `main` | ||
| - name: Check if manifest has newer version | ||
| id: should_release | ||
| run: | | ||
| echo "The manifest version is '$MANIFEST_VERSION' and the released version is '$LATEST_VERSION'" | ||
| HIGHER_VERSION=$(semver "$MANIFEST_VERSION" "$LATEST_VERSION" | tail -n 1) | ||
| if [ "$MANIFEST_VERSION" = "$HIGHER_VERSION" ] && [ "$MANIFEST_VERSION" != "$LATEST_VERSION" ]; then | ||
| echo "🚀 Will release! The manifest version is greater than the released version." | ||
| echo "should_release=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "😴 Holding back from release: The manifest version is not greater than the released version." | ||
| echo "should_release=false" >> "$GITHUB_OUTPUT" | ||
| fi | ||
| shell: bash | ||
|
|
||
| # Here we download the packaged extension artifact to release | ||
| - uses: actions/download-artifact@v4 | ||
| if: github.ref_name == 'main' && steps.should_release.outputs.should_release == 'true' | ||
| with: | ||
| name: ${{ inputs.extension-name }}.tar.gz | ||
|
|
||
| # The release tag utilizes both the extension name and semver version | ||
| # to create a unique tag for the repository | ||
| - name: Release tag | ||
| if: github.ref_name == 'main' && steps.should_release.outputs.should_release == 'true' | ||
| id: release_tag | ||
| run: | | ||
| RELEASE_TAG="${{ inputs.extension-name }}@v$MANIFEST_VERSION" | ||
| echo "RELEASE_TAG=$RELEASE_TAG" >> "$GITHUB_ENV" | ||
| shell: bash | ||
|
|
||
| - name: Release | ||
| if: github.ref_name == 'main' && steps.should_release.outputs.should_release == 'true' | ||
| run: | | ||
| gh release create $RELEASE_TAG \ | ||
| --title "${{ inputs.extension-name }} v$MANIFEST_VERSION" \ | ||
| ${{ inputs.extension-name }}.tar.gz | ||
| shell: bash | ||
|
|
||
| # We fetch the GitHub release using the GitHub API, storing the data | ||
| # for use in the extension list update. | ||
| - name: Get release data | ||
| if: github.ref_name == 'main' && steps.should_release.outputs.should_release == 'true' | ||
| run: gh api /repos/${{ github.repository }}/releases/tags/$RELEASE_TAG > release-${{ inputs.extension-name }}.json | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More for my education / curiosity: what's in the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great question. It is the response from GitHub's "Get a release by tag name" API endpoint. Using this as opposed to
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah interesting. It might be worth stating (more?) explicitly in the comment "We fetch the full release data from the github API" That wasn't at the top of my head for where we were fetching that from, actually.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the comment in e534088 to mention that the release data is fetched from the GitHub API 👍 |
||
| shell: bash | ||
|
|
||
| # Each release data file is uploaded as an artifact for the extension list update | ||
| # The naming convention is `release-<extension-name>.json` to easily | ||
| # download each artifact matching the pattern | ||
| - name: Upload release data | ||
| if: github.ref_name == 'main' && steps.should_release.outputs.should_release == 'true' | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: release-${{ inputs.extension-name }}.json | ||
| path: release-${{ inputs.extension-name }}.json | ||
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add additional lint around name and folder matching? Or lose name entirely and only rely on folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a good idea. I'll review the workflow to see if this 1-1 match is required before adding a hard linting rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to do this as a follow-up for improved linting.