-
Notifications
You must be signed in to change notification settings - Fork 14
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
chore(PPDSC-2611): fewer tests for baseline updates #588
Conversation
You can preview these changes on: |
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.
Nice! Tests are especially helpful.
const data = url.includes('/api/v1/builds') ? BUILDS : SNAPSHOTS; | ||
streamStream.emit('data', JSON.stringify(data)); | ||
streamStream.emit('end'); | ||
}; |
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.
This is nice. I've only seen people using a npm packages. Didn't know this simple but effective solution
echo "Baseline updates required; continuing job." | ||
} | ||
export PR_BRANCH_NAME=$(gh pr list --search $CIRCLE_SHA1 --state merged --json headRefName --jq '.[].headRefName') | ||
echo "Found branch name: ${PR_BRANCH_NAME}" |
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.
Is is worth adding a comment so that people know this env variable name
PERCY_${project.toUpperCase()}_TOKEN
is linked to the comps
passed here?
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.
Good point, perhaps it's not that clear. Looking at it, I think we should just set the PERCY_TOKEN
value here and the script should assume that this is already set (I can comment that this is the case). The only reason the comps
arg is passed is for setting the token.
What do you think?
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.
Implemented
* fix(PPDSC-2611): Revert "Fix/ppdsc 2611 fix 1 (#629)" This reverts commit 7ee4a68. * fix(PPDSC-2611): Revert "fix(PPDSC-2611): fix pipeline (#596)" This reverts commit 1c001a9. * fix(PPDSC-2611): Revert "chore(PPDSC-2611): fewer tests for baseline updates (#588)" This reverts commit fd4d458.
PPDSC-2611
What
include
settingI have done:
I have tested manually:
Before:
After:
Who should review this PR:
How to test: