Skip to content

Commit

Permalink
Adds a new check for required field changes (#2438)
Browse files Browse the repository at this point in the history
* Adds list required field CLI

* Adds new required field check

* Revert test setup

* update if condition

* refactor code

* Update .github/REQUIRED_FIELD_WARNING_TEMPLATE.md

Co-authored-by: Brennan Gamwell <[email protected]>

* Update .github/REQUIRED_FIELD_WARNING_TEMPLATE.md

Co-authored-by: Brennan Gamwell <[email protected]>

---------

Co-authored-by: Brennan Gamwell <[email protected]>
  • Loading branch information
varadarajan-tw and brennan authored Sep 26, 2024
1 parent 04d2827 commit 5a5f52b
Show file tree
Hide file tree
Showing 3 changed files with 260 additions and 0 deletions.
12 changes: 12 additions & 0 deletions .github/REQUIRED_FIELD_WARNING_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<!--REQUIRED_FIELD_DIFF-->

# New required fields detected

> [!WARNING]
> Your PR adds new required fields to an existing destination. Adding new required settings/mappings for a destination already in production requires updating existing customer destination configuration. Ignore this warning if this PR is for a new destination with no active customers in production.
The following required fields were added in this PR:

{{FIELDS_ADDED}}

Add these new fields as optional instead and assume default values in `perform` or `performBatch` block.
140 changes: 140 additions & 0 deletions .github/workflows/required-field-check.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
name: Required Field Check

on:
pull_request:
types: [opened, synchronize, reopened]

jobs:
required-field-check:
runs-on: ubuntu-20.04

timeout-minutes: 5

steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
fetch-depth: 0

- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node-version }}
registry-url: 'https://registry.npmjs.org'
cache: yarn

- name: Install Dependencies
run: yarn install --frozen-lockfile

- name: Get files changed in the PR
id: get_files
uses: actions/github-script@v7
with:
script: |
const response = await github.request('GET /repos/{owner}/{repo}/pulls/{pull_number}/files', {
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number
})
const files = response.data.map(file => file.filename)
core.setOutput('files', files.join(' '))
if (files.length === 0) {
core.setOutput('no_files_changed', true)
}
- name: Find required fields on current branch
id: required-fields-curr-branch
if: steps.get_files.outputs.no_files_changed != 'true'
run: |
REQUIRED_FIELDS_CURR_BRANCH=$(./bin/run list-required-fields -p ${{ steps.get_files.outputs.files }} | jq -c .)
echo "REQUIRED_FIELDS_CURR_BRANCH=$REQUIRED_FIELDS_CURR_BRANCH" >> $GITHUB_ENV
- name: Check for required fields on main branch
id: required-fields-main-branch
if: steps.get_files.outputs.no_files_changed != 'true'
run: |
git checkout main
REQUIRED_FIELDS_MAIN_BRANCH=$(./bin/run list-required-fields -p ${{ steps.get_files.outputs.files }} | jq -c .)
echo "REQUIRED_FIELDS_MAIN_BRANCH=$REQUIRED_FIELDS_MAIN_BRANCH" >> $GITHUB_ENV
- name: Checkout the PR branch again
if: steps.get_files.outputs.no_files_changed != 'true'
run: git checkout ${{ github.head_ref }}

- name: Add comment on PR if there is diff in required fields
if: steps.get_files.outputs.no_files_changed != 'true'
uses: actions/github-script@v7
with:
script: |
const fs = require('fs')
const path = require('path')
const fieldsAdded = []
if(process.env.REQUIRED_FIELDS_CURR_BRANCH && process.env.REQUIRED_FIELDS_MAIN_BRANCH) {
const requiredFieldsOnBranch = JSON.parse(process.env.REQUIRED_FIELDS_CURR_BRANCH)
const requiredFieldsOnMain = JSON.parse(process.env.REQUIRED_FIELDS_MAIN_BRANCH)
Object.keys(requiredFieldsOnBranch).forEach(key => {
// Check if key is present in requiredFieldsOnMain
if(requiredFieldsOnMain[key]) {
const getActionKeys = Object.keys(requiredFieldsOnBranch[key])
for(const actionKey of getActionKeys) {
const branchRequiredFields = requiredFieldsOnBranch[key][actionKey]
const mainRequiredFields = requiredFieldsOnMain[key][actionKey]
const diff = branchRequiredFields.filter(field => !mainRequiredFields?.includes(field))
if(diff.length > 0) {
const actionOrSettings = actionKey === 'settings' ? '**Setting**' : `**${actionOrSettings}**:${actionKey},`
fieldsAdded.push(`- **Destination**: ${key}, ${actionOrSettings} **Field(s)**:${diff.join(',')}`)
}
}
} else {
// If key is not present in requiredFieldsOnMain, then all fields are added recently
const getActionKeys = Object.keys(requiredFieldsOnBranch[key])
for(const actionKey of getActionKeys) {
const branchRequiredFields = requiredFieldsOnBranch[key][actionKey]
const actionOrSettings = actionKey === 'settings' ? 'Settings' : 'Action'
fieldsAdded.push(`- **Destination**: ${key}, **${actionOrSettings}**:${actionKey}, **Fields**:${branchRequiredFields.join(',')}`)
}
}
})
}
// Get the comments of the current PR
const comments = await github.rest.issues.listComments({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo
})
const diffComment = comments.data.find(comment => comment.body.includes('<!--REQUIRED_FIELD_DIFF-->'))
const comment = fs.readFileSync("./.github/REQUIRED_FIELD_WARNING_TEMPLATE.md", "utf8")
const commentBody = comment.replace('{{FIELDS_ADDED}}', fieldsAdded.join('\n'))
if (fieldsAdded.length > 0) {
// Check if the comment is already added
if (!diffComment) {
await github.rest.issues.createComment({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
body: commentBody
})
} else {
await github.rest.issues.updateComment({
comment_id: diffComment.id,
owner: context.repo.owner,
repo: context.repo.repo,
body: commentBody
})
}
} else {
if (diffComment) {
await github.rest.issues.deleteComment({
comment_id: diffComment.id,
owner: context.repo.owner,
repo: context.repo.repo
})
}
}
108 changes: 108 additions & 0 deletions packages/cli/src/commands/list-required-fields.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import { Command, flags } from '@oclif/command'
import { loadDestination } from '../lib/destinations'
import globby from 'globby'
import type { DestinationDefinition as CloudDestinationDefinition } from '@segment/actions-core'
import type { BrowserDestinationDefinition } from '@segment/destinations-manifest'

export default class ListRequiredFields extends Command {
public static enableJsonFlag = true

static description = `Returns a list of required fields for each action in the destination.`

static examples = [
`$ ./bin/run list-required-fields`,
`$ ./bin/run list-required-fields -p ./packages/destination-actions/src/destinations/hubspot/index.ts`
]

// eslint-disable-next-line @typescript-eslint/no-explicit-any
static flags: flags.Input<any> = {
help: flags.help({ char: 'h' }),
path: flags.string({
char: 'p',
description: 'file path for the integration(s). Accepts glob patterns.',
multiple: true
})
}

static args = []

async run() {
const { flags } = this.parse(ListRequiredFields)

const globs = flags.path || [
'./packages/*/src/destinations/*/index.ts',
'./packages/browser-destinations/destinations/*/src/index.ts'
]

const files = await globby(globs, {
expandDirectories: false,
gitignore: true,
ignore: ['node_modules']
})

const destinationMap: Record<string, Record<string, string[]>> = {}
const actionToRequiredFields: Record<string, string[]> = {}

const cloudDestinationRegex = new RegExp(/packages\/destination-actions\/src\/destinations\/([^/]+)\/*/i)
const browserDestinationRegex = new RegExp(/packages\/browser-destinations\/destinations\/([^/]+)\/*/i)

for (const file of files) {
let filePath = file
// extract the destination folder name from the file path
const cloudMatch = cloudDestinationRegex.exec(file)
const browserMatch = browserDestinationRegex.exec(file)
if (cloudMatch) {
const destination = cloudMatch[1]
if (destinationMap[destination]) {
continue
}
filePath = `packages/destination-actions/src/destinations/${cloudMatch[1]}/index.ts`
} else if (browserMatch) {
const destination = browserMatch[1]
if (destinationMap[destination]) {
continue
}
filePath = `packages/browser-destinations/destinations/${browserMatch[1]}/src/index.ts`
}

const destination = await loadDestination(filePath).catch((error) => {
this.debug(`Couldn't load ${filePath}: ${error.message}`)
return null
})

if (!destination) {
continue
}

const settings = {
...(destination as BrowserDestinationDefinition).settings,
...(destination as CloudDestinationDefinition).authentication?.fields
}

destinationMap[destination.name] = { settings: [] }
for (const [key, field] of Object.entries(settings)) {
if (field.required) {
destinationMap[destination.name].settings.push(key)
}
}

for (const [actionKey, def] of Object.entries(destination.actions)) {
const action = def as CloudDestinationDefinition['actions'][string]
const requiredFields = Object.entries(action.fields)
.filter(([_, field]) => field.required)
.map(([key, _]) => key)

if (requiredFields.length) {
actionToRequiredFields[actionKey] = requiredFields
}
}

destinationMap[destination.name] = {
...destinationMap[destination.name],
...actionToRequiredFields
}
}

this.log(JSON.stringify(destinationMap, null, 2))
}
}

0 comments on commit 5a5f52b

Please sign in to comment.