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

feat: pass through functions[].schedule property to ZISI #3761

Merged
merged 10 commits into from
Oct 26, 2021

Conversation

netlify-team-account-1
Copy link
Contributor

@netlify-team-account-1 netlify-team-account-1 commented Oct 25, 2021

Related to

This PR passes through the functions[...].schedule field to ZISI.

TODO:

  • put change behind a feature-flag

@netlify-team-account-1 netlify-team-account-1 changed the title Schedule property feat: pass through functions[].schedule property to ZISI Oct 25, 2021
@netlify-team-account-1 netlify-team-account-1 added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Oct 25, 2021
@netlify-team-account-1 netlify-team-account-1 marked this pull request as ready for review October 25, 2021 08:18
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

Looks good! We'll need two additional things:

  1. Add schedule to the list of function configuration properties (https://github.com/netlify/build/blob/main/packages/config/src/functions_config.js#L52-L58)

  2. Add validation to the schedule property (see https://github.com/netlify/build/blob/main/packages/config/src/validate/validations.js) and accompanying tests

packages/build/src/plugins_core/functions/index.js Outdated Show resolved Hide resolved
packages/build/src/core/feature_flags.js Outdated Show resolved Hide resolved
@ehmicky
Copy link
Contributor

ehmicky commented Oct 25, 2021

Not blocking this PR, but more of a general discussion: do you think we should just forward LaunchDarkly flags from @netlify/build to zip-it-and-ship-it? This would remove the need to do this type of PRs each time zip-it-and-ship-it needs a new feature flag.

@eduardoboucas
Copy link
Member

Not blocking this PR, but more of a general discussion: do you think we should just forward LaunchDarkly flags from @netlify/build to zip-it-and-ship-it? This would remove the need to do this type of PRs each time zip-it-and-ship-it needs a new feature flag.

I'm not sure, to be honest. We're doing more than just passing flags through in normalizeFunctionConfig and I think having that level of control is a good thing.

I'd be more in favour of that if zip-it-and-ship-it was always called from Netlify Build, but that's not the case since CLI interacts with it directly. So I think it's good that zip-it-and-ship-it and Netlify Build have their own set of feature flags.

Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

Looks great! I've added a couple of small comments, with the only blocker being the one about the test.

packages/build/tests/core/tests.js Outdated Show resolved Hide resolved
packages/config/src/validate/validations.js Outdated Show resolved Hide resolved
{
property: 'functions.*.schedule',
check: isValidCronExpression,
message: 'must be a valid cron expression (see https://ntl.fyi/cron-syntax).',
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@kodiakhq kodiakhq bot merged commit d3ccdc4 into main Oct 26, 2021
@kodiakhq kodiakhq bot deleted the schedule-property branch October 26, 2021 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants