-
Notifications
You must be signed in to change notification settings - Fork 73
feat: add config flag support for deploy, preview, and upload commands #790
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
feat: add config flag support for deploy, preview, and upload commands #790
Conversation
🦋 Changeset detectedLatest commit: ea348db 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 |
async function populateR2IncrementalCache( | ||
options: BuildOptions, | ||
populateCacheOptions: { target: WranglerTarget; environment?: string } | ||
populateCacheOptions: { target: WranglerTarget; environment?: string; config?: string } |
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.
Could you please extract the type to top level?
(Maybe add a comment here that the cacheChunkSize
is not used)
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.
LGTM, thanks a lot for the PR 🙏
@james-elicx Could you please take a look when you have time
commit: |
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.
Thanks!
@0ta-mjn We finally went with #791 that James created. It makes the CLI more robust and more extensible and has a better UX. We really appreciate the energy you put in this PR, sorry that we end up merging it :( You feature request should now be addressed. |
@vicb I tested 1.6.0 and tried to run |
Hi @alex-all3dp, I've just run the following command:
And the following is the command that we ran in response to that:
So it looks like the config arg is being passed along. Are you able to expand on what you're seeing? |
Are you referring to cache population instead? Do you have an example of the errors? |
Oh I guess the pb might be with @alex-all3dp would you mind creating a new issue with logs? Thanks! |
^if that's confirmed we could add |
Removing |
Based on #761 .
Old: #762