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 new top-level schedule
property
#819
Conversation
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.
Loving these super small PRs, really easy to review ^^
We already have a test for schedule
: https://github.com/netlify/zip-it-and-ship-it/blob/main/tests/main.js#L2241
What does the new test give us? (not blocking obvsly, the more tests the merrier)
The other test was primarily built to assert the creation of a manifest file. I wanted to have a test specific for schedules, which will be useful once we add the in-code annotations. We can either remove the schedule assertions from the test you linked, or we can keep them because it's fine to have that assertion duplicated. What would you prefer? |
This PR currently has a merge conflict. Please resolve this and then re-add the |
@netlify-team-account-1 the review got stale when I fixed the merge conflict - could you ✅ again, please? |
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
- Summary
Currently, any schedule data is surfaced in the result of
zipFunction
andzipFunctions
asconfig.schedule
. This PR adds a new top-levelschedule
property, which contains the same value.This property will be useful when we introduce the in-code config declarations, because we'll be able to merge schedule data coming from the two types of sources and decide which one takes priority.
- Test plan
New test added.