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.

Sign up for GitHub

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

Jump to bottom

chore: #116 #117 update ts-definition pipeline, scripts #226

Merged
merged 5 commits into from
Feb 13, 2020
Merged

chore: #116 #117 update ts-definition pipeline, scripts #226

merged 5 commits into from
Feb 13, 2020

Conversation

phmngocnghia
Copy link
Contributor

Fixes #116 #117

duong-se
duong-se reviewed Feb 11, 2020
View reviewed changes
package.json Outdated
@@ -7,6 +7,8 @@
"scripts": {
"build": "lerna run build:prod --parallel",
"fetch-config": "yarn config-manager getSecret reapit-marketplace-app-config",
"fetch-definition": "node './scripts/foundations-ts-definitions/fetch-definition.js'",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one should be add to foundations-ts-definitions package.json instead of let it in root package.json for specific case

duong-se
duong-se reviewed Feb 11, 2020
View reviewed changes
package.json Outdated
@@ -7,6 +7,8 @@
"scripts": {
"build": "lerna run build:prod --parallel",
"fetch-config": "yarn config-manager getSecret reapit-marketplace-app-config",
"fetch-definition": "node './scripts/foundations-ts-definitions/fetch-definition.js'",
"handle-cron-job": "node './scripts/foundations-ts-definitions/handle-cronjob.js'",
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

willmcvay
willmcvay reviewed Feb 11, 2020
View reviewed changes
Copy link
Contributor

@willmcvay willmcvay left a comment

Choose a reason for hiding this comment

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

Basically looks good, agree with @tanphamhaiduong 's comments, happy to merge if these are addressed.

packages/foundations-ts-definitions/.npmrc
@@ -0,0 +1 @@
//registry.npmjs.org/:_authToken=${NPM_TOKEN}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this is necessary as the npmrc is at the package root

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npmrc is required because even using yarn workspace command to execute the release script, the cwd still resides in the packages/foundation. Yarn/NPM only read npmrc to its closest workspace directory (the nearest folder containing package.json). Having tested this against release:npm script, this could lead to possible bugs of other NPM packages release production flow (elements, cognito... etc)

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 have also informed our team about this. Testing release production flow of NPM package is currently being conducted.

@phmngocnghia phmngocnghia force-pushed the chore/116-117-update-ts-definition-pipeline-scripts branch from de46145 to b8c0071 Compare February 13, 2020 07:40
@github-actions github-actions bot requested review from nphivu414, duong-se and willmcvay and removed request for zlatanpham February 13, 2020 07:40
@github-actions github-actions bot added aml-checklist Relates to Anti-money laundering app geo-diary Relates to GEO Diary app lifetime-legal Relates to Lifetime Legal App marketplace Relates to the Marketplace smb-onboarder Relates to Small Medium Business onboarding app labels Feb 13, 2020
@phmngocnghia phmngocnghia force-pushed the chore/116-117-update-ts-definition-pipeline-scripts branch from b8c0071 to da6d135 Compare February 13, 2020 11:18
willmcvay
willmcvay approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@willmcvay willmcvay left a comment

Choose a reason for hiding this comment

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

LGTM if @tanphamhaiduong is happy

@github-actions github-actions bot added the elements Relates to Elements components and utilities label Feb 13, 2020
@willmcvay willmcvay merged commit 7c49b93 into master Feb 13, 2020
@willmcvay willmcvay deleted the chore/116-117-update-ts-definition-pipeline-scripts branch February 13, 2020 15:13
phmngocnghia pushed a commit that referenced this pull request Feb 14, 2020
@phmngocnghia
chore: #116 #117 update ts-definition pipeline, scripts (#226)
5ec8331 
* chore: #116 #117 update ts-definition pipeline, scripts

* chore: lock version of package "foundation-ts-definition" for all apps. update PR comment

* fix

* update elements package to include foundation definition

* remove password param from test
nphivu414 pushed a commit that referenced this pull request Apr 29, 2020
[CLD-385]Install cypress and add login test cases (#226)
c822aad
nphivu414 pushed a commit that referenced this pull request Apr 29, 2020
@duong-se
Merge pull request #226 from reapit/develop
040a953 
Release 0.5.30
nphivu414 pushed a commit that referenced this pull request Apr 29, 2020
chore: #116 #117 update ts-definition pipeline, scripts (#226)
a97fc45 
* chore: #116 #117 update ts-definition pipeline, scripts

* chore: lock version of package "foundation-ts-definition" for all apps. update PR comment

* fix

* update elements package to include foundation definition

* remove password param from test
@rpt-uk-github rpt-uk-github mentioned this pull request Dec 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannd4 dannd4 Awaiting requested review from dannd4

@trankhacvy trankhacvy Awaiting requested review from trankhacvy

@vuhuucuong vuhuucuong Awaiting requested review from vuhuucuong

@nphivu414 nphivu414 Awaiting requested review from nphivu414

@duong-se duong-se Awaiting requested review from duong-se

@willmcvay willmcvay Awaiting requested review from willmcvay

Assignees

@phmngocnghia phmngocnghia

Labels
aml-checklist Relates to Anti-money laundering app elements Relates to Elements components and utilities geo-diary Relates to GEO Diary app lifetime-legal Relates to Lifetime Legal App marketplace Relates to the Marketplace smb-onboarder Relates to Small Medium Business onboarding app
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TS Definitions should include a timestamp so I can request the correct version for the API
3 participants
@phmngocnghia @willmcvay @duong-se