-
Notifications
You must be signed in to change notification settings - Fork 239
nonspec: tooling for migration to a late-branching strategy, standardized devcontainers #1328
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
base: main
Are you sure you want to change the base?
nonspec: tooling for migration to a late-branching strategy, standardized devcontainers #1328
Conversation
✅ Deploy Preview for slsa canceled.
|
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.
Pull Request Overview
This pull request introduces several new GitHub workflows to support a migration toward a late-branching strategy and standardizes development containers.
- Added a workflow to create a release on branch updates (currently disabled).
- Introduced a Netlify preview deploy workflow (currently disabled).
- Added a workflow for migrating to a late-branching strategy and updated the lint job to reference the new lint script location.
Reviewed Changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
.github/workflows/update-release.yml | Adds a release creation workflow, disabled with an "if: false" flag. |
.github/workflows/netlify-preview.yml | Introduces a Netlify preview deploy workflow, disabled by default. |
.github/workflows/migrate-to-late-branch.yml | Adds a manually triggered migration workflow for late-branching. |
.github/workflows/lint.yml | Updates the lint step to reference the script in the tools folder. |
Files not reviewed (4)
- .devcontainer/devcontainer.json: Language not supported
- .devcontainer/post-create.sh: Language not supported
- tools/create-spec-release.sh: Language not supported
- tools/migrate-to-late-branch.sh: Language not supported
Comments suppressed due to low confidence (1)
.github/workflows/lint.yml:14
- Verify that the new lint script path (./tools/lint.sh) is correct and that the script has executable permissions to prevent potential build failures.
- run: ./tools/lint.sh
The netlify failure is super confusing. I think it's fine -- this is technically nonspec and nothing should have changed.
|
--title "Release $VERSION" \ | ||
--notes "Automated release from branch $GITHUB_REF" \ | ||
--repo "$GITHUB_REPOSITORY" \ | ||
--generate-notes || echo "Release already exists, skipping." |
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.
probably we'd want to delete and replace any existing releases when this happens?
Signed-off-by: Zachariah Cox <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Zachariah Cox <[email protected]>
Signed-off-by: Zachariah Cox <[email protected]>
@@ -11,4 +11,4 @@ jobs: | |||
uses: actions/setup-node@cdca7365b2dadb8aad0a33bc7601856ffabcc48e # v4.3.0 | |||
- run: npm ci --ignore-scripts | |||
- run: npm run lint --silent | |||
- run: ./lint.sh | |||
- run: ./tools/lint.sh |
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.
this just adapts to the relocated script.
fi | ||
|
||
# --- Create a zip file of the spec directory --- | ||
zip -r spec.zip spec |
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.
this script won't work unless we've already migrated the repo. I can remove it from this PR if it's confusing.
types: [opened, synchronize, reopened] | ||
|
||
jobs: | ||
if: false # This disables the entire workflow |
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.
As discussed in the meeting today, it might make more sense for this to just happen on a daily schedule, only from the main branch, and only if there are published changes to releases (or the draft spec).
this workflow tries to do the right then whenever any PR is opened to any branch? complicated.
but it would be nice to keep the preview site feature if we can prevent it from being super difficult to maintain.
"github.copilot-chat", // Chat interface for GitHub Copilot | ||
"github.github-vscode-theme", // GitHub's official VS Code theme | ||
"github.vscode-github-actions", // GitHub Actions support | ||
"github.vscode-pull-request-github" // GitHub Pull Requests and Issues |
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.
these are technically optional. I have no strong preference.
id: netlify_deploy | ||
env: | ||
NETLIFY_AUTH_TOKEN: ${{ secrets.NETLIFY_AUTH_TOKEN }} | ||
NETLIFY_SITE_ID: ${{ secrets.NETLIFY_SITE_ID }} |
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.
I think we looked it up and we don't actually have these. Netlify is configured to pull automatically from the main branch via webhook. cc: @mlieberman85
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.
Pull Request Overview
This PR introduces tooling improvements to support a migration to a late-branching strategy, enhanced development environments via devcontainers, and experimental Netlify preview deployments.
- Adds new devcontainer configurations and related scripts
- Introduces three new CI workflows (release, late branching migration, and Netlify preview)
- Updates lint workflow to reflect new script locations
Reviewed Changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
.github/workflows/update-release.yml | Adds a GitHub release creation workflow (currently disabled) |
.github/workflows/netlify-preview.yml | Introduces a Netlify deploy preview workflow (currently disabled), with a computed variable not used in the deploy command |
.github/workflows/migrate-to-late-branch.yml | Adds a workflow to trigger the late-branching migration script manually |
.github/workflows/lint.yml | Adjusts the lint script path |
Files not reviewed (4)
- .devcontainer/devcontainer.json: Language not supported
- .devcontainer/post-create.sh: Language not supported
- tools/create-spec-release.sh: Language not supported
- tools/migrate-to-late-branch.sh: Language not supported
if [[ "$PWD" != "$REPO_ROOT" ]]; then | ||
echo "Error: Please run this script from the repository root: $REPO_ROOT" | ||
exit 1 | ||
fi |
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.
Can't you just start with cd "$REPO_ROOT"
and be done?
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.
that would probably work -- unclear if errors or fixes are better?
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.
I generally find it annoying when a program tells me I should do something else rather than just doing it. I always think: "what don't you just do it if you know what needs to be done??"
You could print out a message explaining the change of current directory before doing it so that if anything goes wrong the user isn't left disoriented but that's all that's needed in my opinion.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Zachariah Cox <[email protected]>
per discussion today, I think we want to close this PR. We will
|
Given that the website is slsa.dev, should we use that name? It doesn't really matter, just a thought. |
fixes: #742
How to test:
Development Environment Setup:
.devcontainer/devcontainer.json
to configure a development container with Ruby 3.2, Jekyll, and Netlify CLI. It includes VS Code extensions for GitHub integration and sets up port forwarding for the Jekyll server..devcontainer/post-create.sh
to automate gem installation, configure Bundler for local dependencies, and globally install the Netlify CLI.Proposal for how to perform the late branch refactor
.github/workflows/migrate-to-late-branch.yml
to enable manual migration to a late-branching strategy. The workflow runs a script to create version-specific branches from themain
branch.tools/migrate-to-late-branch.sh
to automate the migration of spec versions to individual branches. The script creates a branch for each version, isolates the relevant files, and pushes the changes.Proposal for changes to the devex
.github/workflows/netlify-preview.yml
to deploy pull request previews to Netlify. The workflow builds the site using Jekyll and posts a comment with the preview link to the pull request..github/workflows/update-release.yml
to create GitHub releases automatically when areleases/*
branch is updated. This workflow is currently disabled.tools/create-spec-release.sh
to package thespec
directory into a zip file, optionally generate an attestation, and create a GitHub release for the current branch.Next steps: