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

BREAKING CHANGE(schema): split authentication.fields into inputFieds and outputFields (PDE-5589 ) #972

Conversation

Natay
Copy link
Contributor

@Natay Natay commented Feb 17, 2025

Fixes the issue: authentication.fields are a combination of input fields (e.g. subdomain or API key - which the user needs to enter), and output fields (computed: true - which we expect authentication procedure to return). Coda, Slack

@Natay Natay requested a review from a team as a code owner February 17, 2025 17:44
@@ -32,7 +32,8 @@ module.exports = {

// Define any input app's auth requires here. The user will be prompted to enter
// this info when they connect their account.
fields: [],
inputFields: [],
Copy link
Contributor Author

@Natay Natay Feb 17, 2025

Choose a reason for hiding this comment

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

Bit of a weird scenario,

  • Leaving fields as is ( not replacing with inputFields ) results in this error
    ✔ package size should not change much (70764 -> 70508 bytes) (248ms)
    basic-auth
Validating project locally
┌────────────────────┬─────────────────────────────────────────────────────────────────┬─────────────────────────────────────────────────────────────────────────┐
│ Property           │ Message                                                         │ Links                                                                   │
├────────────────────┼─────────────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────┤
│ App.authentication │ additionalProperty "fields" exists in instance when not allowed │ https://platform.zapier.com/cli_docs/schema#authenticationschema@[16](https://github.com/zapier/zapier-platform/actions/runs/13375926420/job/37355062393#step:5:17).3.0 │
 ›   Error: "https://zapier.com/api/platform/cli/check" returned "400" saying 
 ›   "App must validate against schema before running app checks"

Going to reach out to team on best approach here, I think I may need to leave the "fields" as is and update it's description to "DEPRECATED"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll dig up some older PRs to see how fields are deprecated and do the same here

Copy link
Contributor Author

@Natay Natay Feb 17, 2025

Choose a reason for hiding this comment

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

Okay,

  • Marked "fields" as deprecated following this PR
  • Created a ticket to update example-apps after this MR, from testing I am still getting we Error: "https://zapier.com/api/platform/cli/check" returned "400" if they are updated in this PR. I think this is because the major version needs to be published first but that's a guess.
  • Created ticket to remove "authentication.fields"

@Natay Natay changed the title feat(schema): split authentication.fields into inputFieds and outputFields (PDE-5589 ) feat(schema): breaking change, split authentication.fields into inputFieds and outputFields (PDE-5589 ) Feb 17, 2025
@Natay Natay marked this pull request as draft February 17, 2025 18:54
@Natay Natay force-pushed the PDE-5589_split_authentication_fields_into_input_and_output branch from 0c5e160 to be18f29 Compare February 17, 2025 19:21
@Natay Natay marked this pull request as ready for review February 17, 2025 19:30
@kola-er
Copy link
Contributor

kola-er commented Feb 18, 2025

@Natay Good start. Reading the requirement again, it seems the auth inputFields and outputFields have some non-common properties as below:

  • For the inputFields
    • helpText
    • placeholder
    • choices
    • inputFormat
  • For the outputFields
    • isNoSecret
    • computed (however this would be redundant and can be removed)

Apologies for leading you on to use same for both, but we would need to break the /AuthFieldSchema into /AuthInputFieldSchema and /AuthOutputFieldSchema.

Also before this is merged, we need to update the backend to support both auth inputFields and outputFields in addition to the auth fields.

@Natay
Copy link
Contributor Author

Natay commented Feb 18, 2025

@kola-er thank you!

Would it make sense to include isNoSecret for inputFields as well? Or do we expect things like "api_key" to be in the output only?

  • Yes

Also, should we update legacy-scripting-runner as well?

  • We should update the examples/tests there.

@Natay Natay changed the title feat(schema): breaking change, split authentication.fields into inputFieds and outputFields (PDE-5589 ) BREAKING CHANGE(schema): split authentication.fields into inputFieds and outputFields (PDE-5589 ) Feb 18, 2025
@Natay Natay requested a review from FokkeZB February 19, 2025 15:48
@Natay
Copy link
Contributor Author

Natay commented Feb 19, 2025

Backend MR to support inputFields and outputFields

@@ -519,7 +516,7 @@ class InvokeCommand extends BaseCommand {
);
}
const authData = await this.promptForAuthFields(
appDefinition.authentication.fields,
appDefinition.authentication.inputFields,
Copy link
Member

@eliangcs eliangcs Feb 24, 2025

Choose a reason for hiding this comment

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

zapier invoke is still gaining adoption. It would hurt if it started breaking. So I'd like the invoke command to continue working on previous versions of platform-core for at least some time. Maybe we could do something like:

Suggested change
appDefinition.authentication.inputFields,
appDefinition.authentication.fields || appDefinition.authentication.inputFields,

And add this back at line 336-338:

      if (field.computed) {
        continue;
      }

@Natay Natay closed this Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants