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

Update CI tooling and workflows #7

Merged
merged 8 commits into from
Jul 25, 2024
Merged

Conversation

jorgecuesta
Copy link
Collaborator

@jorgecuesta jorgecuesta commented Jul 17, 2024

  1. Refactor Code Comments and Update Configurations
    To ensure readability and maintainability, various code comments have been redefined for improved understanding and clarity. Configurations in ".prettierrc" and ".eslintrc.js" have been updated to better align with coding standards. A minor modification has been made to "scripts/build.sh" to enhance the clarity of a logging message.

  2. Update .env File Handling and Introduce Docker Environment Settings
    To handle different development environments, separate .env files (.env.development, .env.test, .env.production) have been introduced. Docker commands are now set to run in the environment specified by these .env files. Docker-compose and Dockerfiles, along with script files, have been updated accordingly. Also, codegen scripts for ESLint and Prettier have been improved.

  3. Revamp CI Tooling and Workflows
    In this updated version, Prettier and ESLint have been introduced for an enhanced development workflow. New GitHub workflows that enable running processes locally as well as with Docker, and cleaning up the cache after a PR merge, have been incorporated. The test baseline has been initialized for future test implementations. Outdated GitHub workflows have been removed.
    With these updates, significant improvements are expected in various aspects of the development process, ranging from CI/CD workflows and environment configuration, to comment clarity and coding configuration.

Check the GitHub Actions on a PR that I open as disposable on my repository to just be sure the CI is working as expected.

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (CI)

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@jorgecuesta jorgecuesta added enhancement New feature or request tooling labels Jul 17, 2024
@jorgecuesta jorgecuesta self-assigned this Jul 17, 2024
Copy link
Member

@okdas okdas left a comment

Choose a reason for hiding this comment

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

@jorgecuesta Thank you so much for this PR! I haven't had the opportunity to thoroughly review all the tooling in this repository yet, and it's fantastic to see community members advancing our toolset.

I've left a few questions in the review. Additionally, we need to remove the zip files from this branch before merging.

Unfortunately, there's no ideal way to test the workflows before merging, and I anticipate that not everything will work perfectly on the first attempt. However, this is an excellent starting point, and we can make adjustments as needed in the future.

.github/workflows/ci-docker.yml Outdated Show resolved Hide resolved
.github/workflows/ci-docker.yml Outdated Show resolved Hide resolved
.github/workflows/ci-docker.yml Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
TODO.md Outdated Show resolved Hide resolved
@jorgecuesta
Copy link
Collaborator Author

Please @okdas and @bryanchriswhite take a look at it again because following most of the conversations with @bryanchriswhite I fully reworked all the devex.

Also as @okdas mentioned in a comment, we should take a single path for the CI instead of both (host & docker). Right now, they are both here to demonstrate that we can test in any way with the CI.

@jorgecuesta jorgecuesta requested a review from okdas July 19, 2024 02:04
package.json Outdated Show resolved Hide resolved
@bryanchriswhite
Copy link
Contributor

image

package.json Outdated
"docker:ps:production": "yarn run docker:compose production ps",
"docker:stop:production": "yarn run docker:compose production stop",
"docker:clean:production": "yarn run docker:compose production down -v",
"docker:tunnel:production": "yarn run docker:check-env:production && ./scripts/proxy-tunnel.sh production",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reduce docker:tunnel to a single yarn script?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thing is how it will do the proper checks using the right docker-compose file and dotenv without noticing the environment he is trying to reach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bryanchriswhite I get it done on the latest commit, please review it again and let me know.

@bryanchriswhite
Copy link
Contributor

Is there a reason we can't use a single docker-compose.yml?

@jorgecuesta
Copy link
Collaborator Author

jorgecuesta commented Jul 19, 2024

Is there a reason we can't use a single docker-compose.yml?

every environment has significant differences:

  1. Production uses node.dockerfile and shares the services with dev but does not mount paths
  2. Development use dev-node.dockerfile and share services with prod but mount paths for hot-reload
  3. Test share image with dev but do not need all the services and also mount paths like dev too.

So to avoid commit .env. files which is an ugly practice, exists just a .env.sample so to avoid multiple docker-compose.yml files we need to run more under the hood scripts to prefill on every .env. the proper dockerfile they will be using.

@Olshansk
Copy link
Member

images

jorgecuesta and others added 7 commits July 22, 2024 18:20
- Add Prettier
- Add Eslint
- Add CI GitHub workflows:
  - Add On Host CI (run things locally)
  - Add On Docker CI (run things with docker)
  - Add Cache Cleanup after PR merge
- Add baseline for Test (but are not working)
- Remove env variable `WATCH` in favor of using `NODE_ENV` as the driver for dependencies and execution simplifying DevEx
- Remove old GitHub Workflows
Added separate dotenv files (.env.development, .env.test, .env.production) for different environments. Docker commands run in the environment specified by these dotenv files. Also, updated Docker-compose, Dockerfiles, and script files to accommodate these changes. Improved codegen scripts for ESLint and Prettier.
This commit enhances various code comments for improved understanding and clarity. Also, configurations have been updated in ".prettierrc" and ".eslintrc.js" to better align with coding standards. Furthermore, a minor modification to "scripts/build.sh" improves the clarity of a logging message.
…ration

Streamlined environment configuration for better management and readability. Refactored package.json scripts to use new script files and clearly define environment related tasks. Additionally, updated Docker setup and distribute new configuration across different compose and Dockerfile files for more explicit handling of development, test, and production environments.
This commit includes several changes, primarily focusing on updating CI workflows and improving Docker setup. CI workflows were updated to optimize the docker building process by using matrix strategies and updating dependencies. Moreover, changes are made to package.json and docker-compose scripts to ensure smooth interaction with Docker and to introduce new environment variables for better control. Finally, the tunnel setup and cleanup for the proxy between the localnet and Docker have been improved for better development experience.
The commit removes specific environment (env) details from the Continuous Integration (CI) job's name in the Docker workflow file. This simplifies the job name, providing a more general and cleaner view in the workflow jobs dashboard.
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

It's hard to grasp all the changes here but looks like a lot of great work overall!

Just a few comments, clarifications and requests to potential README updates. Should be g2g after

src/test/mappingHandlers.test.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docker/node.dockerfile Outdated Show resolved Hide resolved
scripts/docker-compose.sh Show resolved Hide resolved
scripts/node-entrypoint.sh Outdated Show resolved Hide resolved
scripts/watch-exec.sh Outdated Show resolved Hide resolved
scripts/proxy-tunnel.sh Outdated Show resolved Hide resolved
scripts/prepare-dotenv.sh Show resolved Hide resolved
Updated Dockerfiles to use Node.js 22.5, removing outdated comments from several scripts. Enhanced the README with details on available scripts and improved clarity of instructions.
@Olshansk
Copy link
Member

Olshansk commented Jul 25, 2024

Going to merge this in and we'll fix things along the way as they come up.

@jorgecuesta Appreciate your quick turnarounds and responsiveness.

@Olshansk Olshansk merged commit 49d11b4 into pokt-network:main Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tooling] Setup Github actions CI
4 participants