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.

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

Merged
merged 1 commit into from
Feb 14, 2024
Merged

Adding initial NX support #217

merged 1 commit into from
Feb 14, 2024

Conversation

apinkert
Copy link
Contributor

@apinkert apinkert commented Jan 2, 2024

Moving into code review to get some eyes while getting the release stage in GH actions working

@apinkert apinkert requested review from a team and removed request for a team January 30, 2024 23:54
@apinkert apinkert marked this pull request as ready for review January 30, 2024 23:55
@Hyperkid123
Copy link
Contributor

Hyperkid123 commented Jan 31, 2024

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.

Copy link
Contributor

@Hyperkid123 Hyperkid123 left a 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.

"rules": {
"body-max-line-length": [0]
}
}
Copy link
Contributor

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

Copy link
Contributor Author

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

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 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"
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 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",
Copy link
Contributor

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",
Copy link
Contributor

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

@@ -57,7 +57,10 @@ jobs:
- uses: actions/checkout@v3
with:
fetch-depth: 0
ssh-key: ${{ secrets.GH_BOT_TOKEN }}
Copy link
Contributor

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 }}
Copy link
Contributor

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.

@apinkert
Copy link
Contributor Author

apinkert commented Feb 9, 2024

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

@Hyperkid123 Hyperkid123 merged commit d9d03f9 into master Feb 14, 2024
8 checks passed
@Hyperkid123 Hyperkid123 deleted the nx-migration branch February 14, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants