-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix link in CONTRIBUTING.md #55
Conversation
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.
Ah good catch! I now updated the branch protection ruleset to enforce all checks to succeed before PRs can be merged (should've done this after adding the xrefcheck workflow).
The link still isn't right though, I made a suggestion :)
CONTRIBUTING.md
Outdated
@@ -65,5 +65,5 @@ Integration tests are declared in [`./tests`](./tests) as subdirectories imitati | |||
|
|||
Pinned dependencies are [regularly updated automatically](./.github/workflows/update.yml). | |||
|
|||
Releases are [automatically created](./.github/workflows/release.yml) when the `version` field in [`Cargo.toml`](./Cargo.toml) | |||
Releases are [automatically created](./.github/scripts/release.sh) when the `version` field in [`Cargo.toml`](./Cargo.toml) |
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.
Releases are [automatically created](./.github/scripts/release.sh) when the `version` field in [`Cargo.toml`](./Cargo.toml) | |
Releases are [automatically created](./.github/workflows/after-release.yml) when the `version` field in [`Cargo.toml`](./Cargo.toml) |
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.
Oh I see that you intentionally changed it to scripts/release.sh
. I think pointing to the workflow is better, since that's what causes the script to run automatically (the script by itself could also be run manually).
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 thought main.yml
caused release.sh to run?
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.
Oh damn you're right! I take it back, release.sh
seems much better to point to now 😆
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.
Lol I clicked auto-merge and it merged it before we could fix it back.
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.
Opened #57 to fix this, no worries ;)
83a1b57
to
96c5efc
Compare
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.
Force pushed the suggestion.
Thought: It's unfortunate that we can't really use GitHub's web interface to create fixup commits, since we enforce merge commits, so we can't squash those. Maybe we should switch to not requiring merge commits? I do otherwise still like the idea of using merge commits, but I'm willing to be convinced otherwise!
I'm still on team "merge commit." Fixups and force-pushing are a fine workflow, or we could be less bothered by one more commit in the merge if created by the WebUI. |
I didn't notice xref was failing for a valid reason when merging #54
Decided to point at the release.sh which publishes the release.