Skip to content
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

next major version #855

Merged
merged 53 commits into from Feb 26, 2024
Merged

next major version #855

merged 53 commits into from Feb 26, 2024

Conversation

travi
Copy link
Member

@travi travi commented Jan 27, 2024

  • drop support for node v16
  • upgrade to probot v13

BREAKING CHANGES:

Copy link

vercel bot commented Jan 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 26, 2024 4:45am

@travi travi changed the title feat(node-versions): dropped support for node v16, v17, and v19 next major version Jan 27, 2024
julekirk and others added 8 commits February 19, 2024 16:04
Co-authored-by: May Liang <LiangMay@JohnDeere.com>
Co-authored-by: Matthew Travi <TraviMatthewJ@JohnDeere.com>
Co-authored-by: May Liang <LiangMay@JohnDeere.com>
Co-authored-by: Matthew Travi <TraviMatthewJ@JohnDeere.com>
Co-authored-by: May Liang <LiangMay@JohnDeere.com>
Co-authored-by: Matthew Travi <TraviMatthewJ@JohnDeere.com>
BREAKING CHANGE: probot v13 includes breaking changes. see the probot
release notes for details:
https://github.com/probot/probot/releases/tag/v13.0.0

Co-authored-by: Julie Van Kirk <VanKirkJulieA@JohnDeere.com>
Co-authored-by: May Liang <LiangMay@JohnDeere.com>
travi and others added 2 commits February 21, 2024 11:28
Co-authored-by: Julie Van Kirk <VanKirkJulieA@JohnDeere.com>
Co-authored-by: May Liang <LiangMay@JohnDeere.com>
@travi
Copy link
Member Author

travi commented Feb 22, 2024

we will need to coordinate handling the breaking webhook path change and the vercel pre-parsing for the hosted instance before merging this, but i think the changes are otherwise ready to be promoted

@travi travi marked this pull request as ready for review February 22, 2024 17:53
Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me 👍🏼

the vercel pre-parsing for the hosted instance before merging this

yeah I had problems with that as well, I couldn't figure out how to programmatically disable it, only using the environment variable. But I also didn't spend much time on investigating. Definitely do a pre-deployment and test it first before

@travi
Copy link
Member Author

travi commented Feb 25, 2024

I couldn't figure out how to programmatically disable it, only using the environment variable

since we publish this app as a package, i think it makes sense to limit the directive to disable to the vercel deployment, so the environment variable approach makes sense to me for now.

i could see value in having a separate repo at some point in the future that is dedicated to installing the package and deploying to vercel. that would enable the programmatic approach making more sense and also enabling us to release a package version separately from deploying in case there are ever vercel specific issues that we need to handle separately. hopefully that is rare, but i like the ability to handle that and also eat our own dog food by consuming the package.

Definitely do a pre-deployment and test it first before

i'll test out the preview for this beta branch

@travi
Copy link
Member Author

travi commented Feb 25, 2024

i'll test out the preview for this beta branch

i got a test app instance and used it for the preview deployed to the beta branch. without the secrets defined for that instance, the ping webhook was failing the signature check. after defining and re-deploying, that webhook was processing correctly.

the breaking webhook path change

looks like our config was already using this path, so i see no change necessary. i assume that was able to be configured that way before because of it being a serverless deployment?

vercel pre-parsing

i configured the NODEJS_HELPERS variable for both the preview instance and the production instance. i've seen no errors in the logs for either instance after getting the signature piece sorted out. i think it is safe to leave it defined for production, but i havent tested a webhook against the the preview instance other than the ping yet (which wouldnt actually exercise the app) and need to step away. if we think we need to exercise that, we'll need to set up a quick test repo and make sure the preview instance is installed there

@travi
Copy link
Member Author

travi commented Feb 25, 2024

i was able to get a test repo in place and wired up to the preview installation, but it is resulting in 401 responses for actual changes, so working on tracking down what is resulting in that

@travi
Copy link
Member Author

travi commented Feb 25, 2024

looks like our config was already using this path, so i see no change necessary. i assume that was able to be configured that way before because of it being a serverless deployment?

forgot the details of the vercel config that we are using based on https://probot.github.io/docs/deployment/#vercel, so that should be good to go

@travi
Copy link
Member Author

travi commented Feb 26, 2024

it is resulting in 401 responses for actual changes, so working on tracking down what is resulting in that

i got this resolved and everything appears to be working as expected. i think this is ready to merge

@travi travi requested a review from gr2m February 26, 2024 04:50
@travi travi merged commit 8e5e727 into master Feb 26, 2024
10 checks passed
@travi travi deleted the beta branch February 26, 2024 04:53
Copy link

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants