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

jenkins,win: add buildpulse support #3653

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

StefanStojanovic
Copy link
Contributor

This commit adds initial BuildPulse support by optionally uploading test results there. If the machine is not configured to connect to BuildPulse, or if any condition is not met, the data will not be sent. If the upload fails, the test job will just continue.

For now, this is a PoC and only Windows machines can upload results. If we decide to move forward with it, other platforms will be added.

The current situation in the CI is that only the recently added Windows 10 and Windows 11 machines (8 in total) are configured to send the data. There is also this copy of node-test-commit-windows-fanned which I used to test the flow while working on these changes.

Once this lands, we'll start gathering results from all CI runs (PRs and daily builds) and soon, we'll see a part of what BuildPulse can bring to the table. The changes are safe as they enable optional behavior that cannot fail the CI job. The idea is to let this run for some time (BuildPulse keeps data for 14 days) until we can see some meaningful data. The dashboard can be seen here, and I'm sure @siddhantdange can help with accessing it if someone has an issue with that. After that, it can be further discussed if this will move forward and how. If not, we can even drop this commit to keep the build repo cleaner.

cc @nodejs/build

Refs: #3575

@MoLow
Copy link
Member

MoLow commented Mar 19, 2024

I am -1 on adding additional flakiness detection systems. we already have https://github.com/nodejs/reliability.
the challenge is finding the root cause of the flakiness and fixing it

This commit adds initial BuildPulse support by optionally uploading
test results there. If machine is not configured to connect to
BuildPulse, or if any condition is not met, the data will not be sent.
If the upload fails, the test job will just continue.

For now, this is a PoC and only Windows machines can upload results.
If we decide to move forward with it, other platforms will be added.

Refs: nodejs#3575
@StefanStojanovic
Copy link
Contributor Author

Based on the discussion in #3575 there is a consensus to try this. All that was left was for someone to volunteer, which I did. Adding this doesn't mean dropping the reliability repo, nor does it interfere with fixing the tests. This is just another tool that could help us, thus the PoC first to understand the potential it has. If after the evaluation period, we decide not to go forward with it, I have no objections to dropping it, but since I've already invested time in this and everything is ready for PoC it'd be good to at least try it out.

@targos
Copy link
Member

targos commented Mar 20, 2024

Is there a risk of leaking sensitive information when we run CI for security releases?

@StefanStojanovic
Copy link
Contributor Author

I'm unsure how running CI for security release works, but I assume there is a private nodejs/node fork for implementing, reviewing, and landing those changes. If that is correct, CI will be run with an organization different than nodejs. If you look at this part of code, you can see that I prohibit sending results for anything not coming from nodejs/node.

As I mentioned I am not sure how running CI for security release works, so if this doesn't prevent possible leaking of sensitive information, let me know.

@siddhantdange
Copy link

Hi everyone, happy to answer any questions or address concerns. Regarding security, BuildPulse (SOC 2 compliant) only requires test results (junit xml), and various git metadata (commit SHA, tree SHA, branch name, etc) - we do not require or store any source code.

@targos
Copy link
Member

targos commented Mar 21, 2024

I'm unsure how running CI for security release works, but I assume there is a private nodejs/node fork for implementing, reviewing, and landing those changes. If that is correct, CI will be run with an organization different than nodejs. If you look at this part of code, you can see that I prohibit sending results for anything not coming from nodejs/node.

As I mentioned I am not sure how running CI for security release works, so if this doesn't prevent possible leaking of sensitive information, let me know.

Sounds good. Security patches are tested from the nodejs-private org.

@targos
Copy link
Member

targos commented Mar 21, 2024

How are we going to define the buildpulse secret variables?

@mhdawson
Copy link
Member

@StefanStojanovic when we do security releases we lock down the regular CI to do the testing. Because of that any data exported from the regular CI duing that time could leak info.

@StefanStojanovic
Copy link
Contributor Author

@mhdawson, since the security repo is in a node-private organization, it will not be uploaded to BuildPulse. We only send results for nodejs/node builds.

How are we going to define the buildpulse secret variables?

Currently, in the PoC, they are set as environment variables in the configured machines. Before I go any further, I know this is extremely bad, as they can be seen from build in Jenkins, but I discussed it with @siddhantdange and those keys only give write access to certain S3 buckets (to upload results). Also, those secrets are connected to my account, which is not even connected to nodejs/node but my fork. The reason I did it like this is simply speed and nothing else. If we decide to go forward with it, I plan to use Jenkins to store those secrets, similar to how we access temp repo (eg. secrets text/file). Is this good approach or should it be done differently?

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

Can't review the code but I approve the experiment.

@StefanStojanovic
Copy link
Contributor Author

StefanStojanovic commented Mar 26, 2024

Since, after discussion, there were no objections to this, I'll land it tomorrow morning and monitor CI carefully in the following days to make sure this doesn't break anything.

@StefanStojanovic StefanStojanovic merged commit 11ee0af into nodejs:main Mar 27, 2024
1 check passed
StefanStojanovic added a commit to JaneaSystems/build that referenced this pull request Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants