-
Notifications
You must be signed in to change notification settings - Fork 48
Adding basic CICD, commenting out old CICD, disabling nx cloud #111
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
base: main
Are you sure you want to change the base?
Conversation
maxn990
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.
approved with a few comments, overall looks really good! maybe we could add some sort of workflows.md file too just to clarify the structure for new tls and also explain why some parts are commented out.
thanks for taking this on, it's much appreciated!
| # name: Legacy CI/CD | ||
|
|
||
| # # First runs linter and tests all affected projects | ||
| # # Then, for each project that requires deployment, deploy-<project>-<service-type> is added | ||
| # # Environment variables are labelled <project-name>_SHORT_DESCRIPTION |
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.
could we make it clear (in comments) that this is an example workflow for deployment, i think it might be confusing potentially to new tls creating new projects from scaffolding why all this is commented out. i agree that commenting it out but keeping it is the best way to go though
| @@ -0,0 +1,27 @@ | |||
| name: Linter run | |||
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 there a reason to get rid of nx affected within the linter?
thaninbew
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.
Overall looks good! I also disabled Nx cloud in the FCC repo.
my review from looking at your file changes:
Agreed with Max's points:
- adding clarity to documentation will be best, especially if scaffolding's gonna continue to be the norm for new C4C projects. "make it clear (in comments) that this is an example workflow for deployment"
- I also think that we should keep nx affected if there wasn't any specific reason for removing it, since our projects can get large and linter can get slow.
Additional:
- workflow files, (i.e.
backend_build.ymland others..) should have trailing newlines at the end (I also lowkey forget, but it's best practice) - added a comment on the
lint.ymlfile
| branches: ['main'] | ||
|
|
||
| jobs: | ||
| build: |
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.
should this be "lint:" ?
📝 Description
CICD from BHCHP that builds frontend using
npx nx build frontendandnpx nx build backendand uses yarn to install and run dependencies.Disables NX cloud so that GitHub Actions workloads don't become limitted.
Frontend build, backend build, linter, and test run are all separate so that there can be clear specificity from the PR page of what failed.
Comments out old CICD, saving it if a team has fully build-out cloud infrastructure for frontend/ backend using app runner and amplify. This CICD is more suitable for projects because it runs everything locally as it would be run locally.
✔️ Verification
Works on the BHCHP repo.