-
Notifications
You must be signed in to change notification settings - Fork 25
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
Adding initial NX support #217
Conversation
6932a0e
to
0a94940
Compare
About the release job. First, there needs to be a couple of tags so the scripts have some base. I can give you more details in PMs. The release job should be also disabled on PRs There also seem to be quite a lot of logging happening caused by the maven installation. Is there a way to "silence them"? Its going to be a pain to find errors in the logs when some occur. We could also cache the maven packages (if possible) I don't expect the Java dependencies to change that often really. |
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.
There is a lot of JS dependencies that are not required for this repo.
.commonlintrc.json
Outdated
"rules": { | ||
"body-max-line-length": [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.
You are missing new line at the end of most of these files
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.
Looking into why this isn't getting caught
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 think we are simply not linting the JSON files.
package.json
Outdated
"build": "nx run-many -t build", | ||
"lint": "nx run-many -t lint", | ||
"lint:fix": "nx run-many -t lint --fix", | ||
"version": "nx run-many -t version --dryRun" |
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.
Don't think you want the --dryRun
flag here
package.json
Outdated
"@emotion/react": "11.11.1", | ||
"@emotion/styled": "11.11.0", | ||
"@mui/icons-material": "^5.14.19", | ||
"@mui/material": "^5.14.19", |
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 don't think we need the @mui packages. Nor any other react packages.
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@redhat-cloud-services/vulnerabilities-client", | |||
"version": "1.2.13", | |||
"version": "1.2.14", |
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.
Last thing before release. We will have to make sure, all the tags, releases and actual npm package versions are in sync. For example this version does not exist on npm:
https://www.npmjs.com/package/@redhat-cloud-services/vulnerabilities-client?activeTab=versions
.github/workflows/ci.yml
Outdated
@@ -57,7 +57,10 @@ jobs: | |||
- uses: actions/checkout@v3 | |||
with: | |||
fetch-depth: 0 | |||
ssh-key: ${{ secrets.GH_BOT_TOKEN }} |
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 can't really inspect the secret, but my gut tells me its not an SSH key of our bot.
npm_token: ${{ secrets.NPM_TOKEN }} |
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.
We do not have this secret configured right now. I'll add it to the secrets.
For an update on this PR, currently waiting for a ticket to create an email for nacho-bot to be used to sign commits/potentially publish npm packages for us. Once that final secret gets setup for the release GH action, I'll clean up this PR and squash all the test version commits |
7cebcb2
to
f744511
Compare
Moving into code review to get some eyes while getting the release stage in GH actions working