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

Initial version from github actions workflow #45

Merged
merged 19 commits into from
Aug 13, 2024

Conversation

fmacleal
Copy link
Contributor

In order to be able to easily have more external contributors, we need to migrate some private pipelines running on jenkins to Github Actions.

This PR starts this migration, executing the steps necessary to run the integration tests between powpeg-node and rskj inside Github actions.

@fmacleal fmacleal force-pushed the feature/add-integration-tests-with-github-actions branch 2 times, most recently from 2320280 to 10c4875 Compare June 10, 2024 20:27
.idea/.gitignore Outdated Show resolved Hide resolved
@fmacleal fmacleal force-pushed the feature/add-integration-tests-with-github-actions branch 23 times, most recently from aa0e256 to 30678ff Compare June 30, 2024 15:51
@fmacleal fmacleal marked this pull request as ready for review July 1, 2024 14:35
@fmacleal fmacleal requested a review from a team as a code owner July 1, 2024 14:35
@marcos-iov
Copy link
Collaborator

pipeline:run

@marcos-iov marcos-iov force-pushed the feature/add-integration-tests-with-github-actions branch from 015fa88 to 59502c2 Compare August 7, 2024 12:54
Copy link
Member

@bcodesido bcodesido left a comment

Choose a reason for hiding this comment

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

LGTM

@marcos-iov
Copy link
Collaborator

pipeline:run

3 similar comments
@marcos-iov
Copy link
Collaborator

pipeline:run

@marcos-iov
Copy link
Collaborator

pipeline:run

@marcos-iov
Copy link
Collaborator

pipeline:run

fi
POWPEG_VERSION=$1
shift
echo "POWPEG_VERSION received as parameter: $1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo "POWPEG_VERSION received as parameter: $1"
echo "POWPEG_VERSION received as parameter: $POWPEG_VERSION"

The shift changes the value of $1. In particular, given that the script aborts if it doesn't have exacly one parameter, this is accessing an unset variable. Depending on how it's run, that can be an error or a warning.

Comment on lines 31 to 32
RUN export JAVA_HOME="$JAVA_HOME" \
&& export PATH="$JAVA_HOME:$PATH"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a no-op and adds an extra layer to the container. The reach of those export are for the whole RUN instruction, not for all the ones after. My understanding is that the previous ENV JAVA_HOME=... should be enough.

@nathanieliov
Copy link
Contributor

pipeline:run

Comment on lines 6 to 7
&& apt-get install -y --no-install-recommends \
wget gnupg2 curl git ca-certificates mocha \

Check notice

Code scanning / SonarCloud

Arguments in long RUN instructions should be sorted

<!--SONAR_ISSUE_KEY:AZEs6DT14rqB6hKU0Ic1-->Sort these package names alphanumerically. <p>See more on <a href="https://sonarcloud.io/project/issues?id=rsksmart_rootstock-integration-tests&issues=AZEs6DT14rqB6hKU0Ic1&open=AZEs6DT14rqB6hKU0Ic1&pullRequest=45">SonarCloud</a></p>
@fmacleal fmacleal requested a review from lucasvuotto August 8, 2024 17:16
Copy link
Member

@bcodesido bcodesido left a comment

Choose a reason for hiding this comment

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

LGTM

@marcos-iov
Copy link
Collaborator

pipeline:run

3 similar comments
@marcos-iov
Copy link
Collaborator

pipeline:run

@marcos-iov
Copy link
Collaborator

pipeline:run

@nathanieliov
Copy link
Contributor

pipeline:run


## Inputs
By default, all the inputs are pointed to the `master/main` branch of the repositories. But, ideally, we will adapt
the action to receive the branches received to be the one in a branch or tag that we want to test.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This last part is a bit confusing

### `rit-branch`

**Optional** The rootstock-integration-tests branch to checkout. This one it's optional, because it will be
very unlikely that we need to use a different branch for the rootstock-integration-test. It's offered the possibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't say it's very unlikely. Sometimes a new feature requires a change in the RIT so we need to use a specific RIT branch to run the tests along with the feature changes in rskj/powpeg.

Also, when there is a change to the RIT code and a PR against master we also want the tests to run with the RIT branch that has the changes included

Comment on lines 18 to 20
<!-- <filter class="ch.qos.rit-local-configs.classic.filter.ThresholdFilter">
<level>DEBUG</level>
</filter> -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we keep this and other commented out parts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nopes, I will remove it.

Comment on lines 5 to 6
const waitForBlockAttemptTimeMillis = process.env.WAIT_FOR_BLOCK_ATTEMPT_TIME_MILLIS || 200;
const waitForBlockMaxAttempts = process.env. WAIT_FOR_BLOCK_MAX_ATTEMPTS || 160;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tests are failing when executed against this branch. I think it might be related to this, or maybe it's the default values set in configure_rit_locally.sh file.

Could you set them back to the original values please?

.gitignore Outdated
@@ -7,3 +7,4 @@ config/*.js
logs
.env
.DS_Store
.idea/
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should a blank line at the end of this and other files that github is reporting

Copy link

@marcos-iov
Copy link
Collaborator

pipeline:run

2 similar comments
@marcos-iov
Copy link
Collaborator

pipeline:run

@marcos-iov
Copy link
Collaborator

pipeline:run

@marcos-iov marcos-iov merged commit 7cb9f9d into main Aug 13, 2024
7 checks passed
@marcos-iov marcos-iov deleted the feature/add-integration-tests-with-github-actions branch August 13, 2024 16:16
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.