Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

more github actinos #1037

Merged
merged 4 commits into from
Feb 12, 2020
Merged

more github actinos #1037

merged 4 commits into from
Feb 12, 2020

Conversation

EverlastingBugstopper
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper commented Feb 7, 2020

GitHub Actinos status update

Tests now finish in ~4 minutes on cache hits

  • caching cross-platform tests on stable and nightly - every push
  • caching cross-platform debug builds on stable - every push
  • cross-platform release builds and tarballs created - tag pushes that start with v
  • cargofmt - every push
  • create wranglerjs tarball - every push or just tag pushes?
  • create github releases - on release

Open questions:

  • Since this builds a release on tag push, should we create them as pre-releases so that they go live when we say they go live or is it fine the way it is.
  • Do we need to be targeting MacOS 10.7?
  • Do tests compile all of our code/Do we need to build on every push if we're running tests on every push or should we just do the builds for releases?

@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/actinos-pizza-rolls branch 20 times, most recently from 441bf41 to 4639908 Compare February 10, 2020 23:49
@EverlastingBugstopper
Copy link
Contributor Author

this will create a github release when you push a tag that starts with v

Here's a screenshot of my fork's release page:

image

we should double check to make sure that each release binary works as expected.

cc @ashleygwilliams for review

@ashleygwilliams
Copy link
Contributor

Since this builds a release on tag push, should we create them as pre-releases so that they go live when we say they go live or is it fine the way it is.

i feel fine about how it is now, but i'm happy to be persuaded otherwise

Do we need to be targeting MacOS 10.7?

i would focus on the most up-to-date stable macOS only

Do tests compile all of our code/Do we need to build on every push if we're running tests on every push or should we just do the builds for releases?

i think we should just build for releases

this will create a github release when you push a tag that starts with v

the last setup would create a release with any named tag- do we want to change that behavior?

Copy link
Contributor

@ashleygwilliams ashleygwilliams left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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)

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'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?)

Copy link
Contributor

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

Copy link
Contributor

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 👍

if: matrix.build == 'macos'
run: cargo build --release
env:
MACOSX_DEPLOYMENT_TARGET: 10.7
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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!

Copy link
Contributor

@ashleygwilliams ashleygwilliams left a 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

@EverlastingBugstopper EverlastingBugstopper merged commit 621e110 into master Feb 12, 2020
@delete-merged-branch delete-merged-branch bot deleted the avery/actinos-pizza-rolls branch February 12, 2020 22:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants