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

CI/CD quickstart #2777

Merged
merged 7 commits into from
Mar 25, 2024
Merged

CI/CD quickstart #2777

merged 7 commits into from
Mar 25, 2024

Conversation

tishun
Copy link
Collaborator

@tishun tishun commented Mar 11, 2024

Utilizing the Github actions to implement a simple pipeline

Initiated nightly or when there is a push / pull request, it consists of these steps:

  1. Checkout the repo
  2. Set the container environment
  3. Call "make clean"
  4. Call "make test-coverage"

This first implementation heavily relies on the Makefile as a means to automate the process.
It also builds and deploys the Redis instances each time we run the pipeline.

The key disadvantages a future improvement might want to address are:

  • stability - typically client tests are run against a stable release of the server to avoid running into issues with the server itself and not the client code
  • performance - obviously even though the run is fast is executes some steps unnecessarily (if we are able to use a custom Docker image for example we would not have to go through step 2 at all)
  • the "continuous deployment" part of CI/CD is missing, since we do not actually deploy a snapshot to Maven central

The purpose of this initial implementation is to serve as a stepping stone that we can then improve and in the same time guarantee that any incoming changes are not breaking the build integrity or degrading the test coverage.

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

@tishun tishun self-assigned this Mar 11, 2024
run: |
sudo apt update
sudo apt install -y stunnel make git gcc maven
- name: Maven offline
Copy link
Collaborator

@mp911de mp911de Mar 11, 2024

Choose a reason for hiding this comment

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

It would make sense to use the maven setup action:

    - name: Setup Maven Action
      uses: s4u/[email protected]
      with:
        java-version: 8
    - name: Cache Maven
      uses: actions/cache@v3
      with:
        path: ~/.m2
        key: ${{ runner.os }}-maven

to include Maven repo caching, avoiding repetitive artifact downloads.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Totally, makes sense to install maven with an action.

I am a bit hesitant on the cache action topic though. Using this key would bean any build on this OS would be cached once and subsequent builds would use this cache, which I am not sure is correct. I think a better approach is the one given in the manual as it would hash the pom.xml file contents.

What do you think?

Copy link
Collaborator Author

@tishun tishun Mar 12, 2024

Choose a reason for hiding this comment

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

@mp911de What about the latest solution with caching included in the setup-java client? The docs mention that the caching is achieved in this way (by hashing the pom.xml) so I think this should suffice?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably sticking to the manual is fine either. Looking at the setup, as pull requests are included in the run, including the deployment step doesn't make sense.

@mp911de
Copy link
Collaborator

mp911de commented Mar 11, 2024

Do you want to have a separate action for snapshot deployment?

@tishun
Copy link
Collaborator Author

tishun commented Mar 11, 2024

Do you want to have a separate action for snapshot deployment?

Good question!
TBH I was avoiding this topic on purpose until I get a more clear idea of how releases will be done / are done.
Generally I would expect that after we merge we have another pipeline that handles that. It could repeat most of the steps for post-verifications and then deploy the snapshot to maven central.

But I'd like to talk to the team to get some more clarity. Do you think we should wait until this discussion is over?

@mp911de
Copy link
Collaborator

mp911de commented Mar 11, 2024

IMO we could reuse the outcome of this job, run mvn -B -DskipITs=false clean deploy jacoco:report instead of make test-coverage.

@tishun
Copy link
Collaborator Author

tishun commented Mar 11, 2024

IMO we could reuse the outcome of this job, run mvn -B -DskipITs=false clean deploy jacoco:report instead of make test-coverage.

Do you mean remove it from this workflow entirely? What would this flow verify then?

@chayim
Copy link

chayim commented Mar 12, 2024

#2747

@tishun
Copy link
Collaborator Author

tishun commented Mar 25, 2024

@tishun tishun merged commit 8c1529a into redis:main Mar 25, 2024
1 check passed
@tishun tishun deleted the topic/tishun/cicdv1 branch March 26, 2024 13:58
@tishun tishun added the type: task A general task label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants