-
Notifications
You must be signed in to change notification settings - Fork 337
Conversation
441bf41
to
4639908
Compare
this will create a github release when you push a tag that starts with Here's a screenshot of my fork's release page: we should double check to make sure that each release binary works as expected. cc @ashleygwilliams for review |
i feel fine about how it is now, but i'm happy to be persuaded otherwise
i would focus on the most up-to-date stable macOS only
i think we should just build for releases
the last setup would create a release with any named tag- do we want to change that behavior? |
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 this all looks good except we're missing clippy in the linters action!
@@ -0,0 +1,19 @@ | |||
name: Linters |
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.
do we want clippy in here?
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.
we currently do not check clippy in CI and have a number of warnings. this would fail right now so i think this is worth tackling separately.
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.
got it! do we have an issue?
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.
#373 sorta
steps: | ||
- uses: actions/checkout@v1 | ||
|
||
- name: Cache Cargo registry |
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 logic is duplicated - is there anyway to pull it out into it's own thing and share it across the actions (not a blocker, just curious)
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'm not sure! i wasn't able to find anything similar to azure pipelines templates but happy to use if it exists (perhaps @signalnerve knows?)
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.
we can refactor, let's get this in and open an issue to investigate? could be a great way for @signalnerve to contribute to wrangler <3
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.
ahh hey i'm sorry i missed this notification! sounds interesting, would be down to collab with @EverlastingBugstopper on this in the future if you're down 👍
.github/workflows/release.yml
Outdated
if: matrix.build == 'macos' | ||
run: cargo build --release | ||
env: | ||
MACOSX_DEPLOYMENT_TARGET: 10.7 |
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.
how'd we pick this number? was it what we used previously?
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 is what was in azure and why i asked if we needed it
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.
got it. i don't think we need it! apple is particularly shitty about what counts as latest stable... based off of what still gets security updates (https://support.apple.com/en-us/HT201222), i think we should aim for 10.14 right now, but this feels like a guess on my part, so i'm open to thoughts you have!
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.
we'll tackle clippy in another issue
6bf9f4a
to
437a603
Compare
GitHub Actinos status update
Tests now finish in ~4 minutes on cache hits
v
Open questions: