-
Notifications
You must be signed in to change notification settings - Fork 698
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
c3 add final commit to pages projects #3769
c3 add final commit to pages projects #3769
Conversation
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5892511639/npm-package-wrangler-3769 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5892511639/npm-package-wrangler-3769 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5892511639/npm-package-wrangler-3769 dev path/to/script.js Additional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5892511639/npm-package-cloudflare-pages-shared-3769 Note that these links will no longer work once the GitHub Actions artifact expires. |
🦋 Changeset detectedLatest commit: 70c9312 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report
@@ Coverage Diff @@
## main #3769 +/- ##
=======================================
Coverage 75.17% 75.17%
=======================================
Files 194 194
Lines 11410 11410
Branches 3013 3013
=======================================
Hits 8577 8577
Misses 2833 2833 |
ae17f02
to
bc35cd8
Compare
Converted to draft as I need to tweak the details in the commit message and figure out how we want to handle monorepos |
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.
It would be nice to have some test coverage for this if it's easy.
The one hurdle thats been on the backlog is fixing git commits in e2e which fails silently atm since there isn't a git user and email configured inside of the newly created project folder. It would be nice if we could somehow add this config in tests mode and then verify the commit was made via git
at the end of a test run.
@@ -61,7 +66,7 @@ export const runPagesGenerator = async (args: C3Args) => { | |||
} | |||
await updatePackageScripts(ctx); | |||
await offerGit(ctx); | |||
await gitCommit(ctx); | |||
await gitCommit(ctx, generatePagesCommitMessage(ctx)); |
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.
For consistency and to accommodate some changes I'd like to make later, it would be better if we kept the signature as is, and did some branching logic inside gitCommit
function itself to generate the commit one way or the other depending on if it's pages or workers.
You can check this condition by evaluating the truthiness of ctx.framework
-- it will be defined for pages apps but not for workers. Ideally, we'd have a dedicated field that gets set early on in the process (i.e. ctx.projectType = 'workers'
) which we could key off of, but we can use ctx.framework
as a close enough proxy for 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.
ah ok thanks, I'll move the change in the function sorry about that 🙂:+1:
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.
Updated: 68474ca
Please have a look and let me know if this looks ok to you 🙂
@jculvey yes, I was wondering of the testing situation... I wasn't sure if the tests are ready for covering this, it sounds like they're not completely there, I don't mind having a look and if it doesn't make this PR go hugely out of scope I don't mind adding them 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.
this is looking good, leaving a few comments...
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.
once more nit, but the rest looks great. please adjust and go ahead and merge
sorry I've still got a few items todo (I've noted them in the PRs description) 🙇 Also... the C3 tests cleanup workflow is failing... it's probably not ok to merge with it is failing 😓 (seems like its missing the |
df02cbd
to
3c4dd78
Compare
e858eb2
to
70c9312
Compare
Closing this in favour of #3776 (which is practically the same PR but created from an internal branch) Sorry for the inconvenience 🙇 |
TODO:
--commit-message
towrangler pages deploy
in case we aren't in a repositorygit = <git version>
in the commit messagewrangler = wrangler@<wrangler version>
in the commit messageTODO after merging this PR:
(mentioning that the potential solution is to check before committing if there are unstaged changes outside
the current directory and in that case notify the user ask the user and ask them if they are ok with C3 generating
the commit)
Fixes # [insert GH or internal issue number(s)].
What this PR solves / how to test:
Associated docs issue(s)/PR(s):
Author has included the following, where applicable:
Reviewer is to perform the following, as applicable:
Note for PR author:
We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label
highlight pr review
so future reviewers can take inspiration and learn from it.