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

Detect changes and type fixes #28

Merged
merged 8 commits into from
Sep 20, 2022
Merged

Detect changes and type fixes #28

merged 8 commits into from
Sep 20, 2022

Conversation

shahnami
Copy link
Member

@shahnami shahnami commented Sep 17, 2022

This PR addresses #25 and #26.

  • Fixed some typings and schema validation
  • Detect changes before calling 'update' (caveat: this does not happen for secrets, as we cannot retrieve values to compare).

Check list:

Copy link
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

LGTM, except for the type checks in _isProvided, which make me a bit nervous. Also, given that we're now removing all non-empty props, can we test that the plugin properly recognizes updates when we change something from null to non-null and vice versa? Eg setting an autotask condition or trigger for a sentinel.

src/cmd/deploy.ts Show resolved Hide resolved
src/utils/index.ts Outdated Show resolved Hide resolved
src/utils/sanitise.ts Outdated Show resolved Hide resolved
src/utils/index.ts Outdated Show resolved Hide resolved
src/utils/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mok0230 mok0230 left a comment

Choose a reason for hiding this comment

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

a few very minor things, but looks great overall!

src/types/docs/function-properties-trigger.md Outdated Show resolved Hide resolved
src/types/docs/function.md Outdated Show resolved Hide resolved
src/utils/index.ts Show resolved Hide resolved
src/utils/sanitise.ts Show resolved Hide resolved
@shahnami shahnami merged commit e917693 into main Sep 20, 2022
@shahnami shahnami deleted the types/adjust-update-types branch September 20, 2022 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants