-
Notifications
You must be signed in to change notification settings - Fork 47
Improve CI/CD & format fixes #233
Conversation
|
New dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No new dependency issues detected in pull request Bot CommandsTo ignore an alert, reply with a comment starting with Pull request alert summary
📊 Modified Dependency Overview:
🚮 Removed packages: @ethersproject/[email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected] |
collins-w
left a comment
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.
LGTM. Thanks.
zeljkoX
left a comment
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.
LGTM
Small comment about package versions.
| }, | ||
| "dependencies": { | ||
| "defender-sentinel-client": "1.44.0", | ||
| "@openzeppelin/defender-sentinel-client": "1.44.0", |
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.
Not sure should we change this as we don't have yet this package published. This would cause examples not to work.
Same applies to other examples.
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.
I want to check if we can migrate the existing packages already from defender-*-client to @openzeppelin/ i'm not sure if it's possible
mok0230
left a comment
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.
Thanks for doing all this! Just a couple items to take a look at
.github/workflows/publish.yml
Outdated
|
|
||
| - name: Publish | ||
| run: | | ||
| yarn publish |
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.
For a release candidate, will this use the next or beta flag (rather than latest)?
If it does use latest, it may cause npm install to cause users to upgrade to a beta version:
package.json
"openzeppelin/defender-admin-client": "^1.44.0",
may use 1.45.0-rc.1 if that were tagged latest. It's possible the -beta overrides this - not sure - but it would be worth checking.
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.
All the publishes ( from github actions) uses latest since we are not explicitly specifying --dist-tag with lerna publish
mok0230
left a comment
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.
Thanks for doing all this! Just a couple items to take a look at
Thanks for reviewing the PR. 🙏 |
zeljkoX
left a comment
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.
Thanks for hard work!
Thanks for reviewing the PR. 🙏🏽 |


Runs CI/CD
@openzeppelin/...When i configure protected tag for some reason pushing tags fail not sure why.
Once this is merged all the packages will be start publishing to
@openzeppelinso we need to update documentation for existing packages to let users know that we are using this going forward or migrate existing packages fromdefender-*-clientto@openzeppelin/defender-*-clientFOLLOW UP
@openzeppelin/defender-*-clientonce the beta version is published.