Skip to content

CI script changes needed for the New CI #218

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

CI script changes needed for the New CI #218

wants to merge 20 commits into from

Conversation

@bensze01
Copy link
Contributor

@bensze01 bensze01 commented Jul 23, 2025 •
edited by gilles-peskine-arm
Loading

This PR contains all the changes needed to get the new CI working with the main branch. Resolves #225.
(Currently it runs off of dev/dgreen-arm/openci-testing)

@bensze01 bensze01 added enhancement New feature or request size-s Estimated task size: small (~2d) priority-medium labels Jul 23, 2025
@bensze01 bensze01 force-pushed the dev/bensze01/openci-testing branch 7 times, most recently from 81d82b5 to e1e1777 Compare August 20, 2025 07:27
@bensze01 bensze01 force-pushed the dev/bensze01/openci-testing branch from e1e1777 to 8d1aab7 Compare September 11, 2025 14:43
@bensze01 bensze01 mentioned this pull request Sep 11, 2025
Merged
@bensze01 bensze01 force-pushed the dev/bensze01/openci-testing branch from 8d1aab7 to aacb082 Compare September 16, 2025 16:25
dgreen-arm and others added 13 commits October 21, 2025 19:49
@dgreen-arm @bensze01
Explicitly set region
a145b98 
Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Do not modify value of docker_ecr on the Open CI
12560c8 
ECRs are not used on the current Open CI, nor is the value of docker_ecr
used in the Open CI codepaths.

Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Include docker.io in the image URL
d583abd 
Use the full URL instead of the shortened form that docker also permits.

Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Use full URL when checking ECR for existing images
9ef7d16 
Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Fake required results on all CIs
04a8e30 
This way we don't need to update the script if we change which CI runs
the merge queues.

Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Use ECR on the new CI
2ace488 
The image name on the new CI corresponds to the public Dockerhub urls.

Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Add mbedtls prefix to node labels on the new CI
61fd118 
Define a new method, common.mbedtls_node() to make the transition
easier. This lets us keep using the same node labels accross the CIs in
most of the codebase.

Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Rename notifications from the new CI
b53058d 
Signed-off-by: Bence Szépkúti <[email protected]>
@dgreen-arm @bensze01
Use new S3 bucket for the new CI
6eb33f0 
Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Do not silence errors during manifest inspection
a595970 
Log the reason docker manifest inspect failed to help debugging.
It may fail in an unexpected way, eg. if dockerhub is rate limiting us.

Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Log in before attempting manifest inspection
d209b9e 
Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Log in to both the ECR and Dockerhub on the new CI
b7e14c8 
Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01
Use manifest inspect on the new CI
a192372 
Signed-off-by: Bence Szépkúti <[email protected]>
@bensze01 bensze01 force-pushed the dev/bensze01/openci-testing branch from aacb082 to a192372 Compare October 21, 2025 17:51
@gilles-peskine-arm gilles-peskine-arm self-requested a review October 31, 2025 16:08
gilles-peskine-arm
gilles-peskine-arm requested changes Oct 31, 2025
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me on visual inspection. I know this is already running live on the new CI. I still want:

  • Better documentation.
  • Some validation that this doesn't break the old CI while we're transitioning.

vars/common.groovy Outdated
@Field final boolean is_open_ci_env = env.JENKINS_URL ==~ /\S+(mbedtls\.trustedfirmware\.org)\S+/

/* Indicates if CI is running on the new CI (hosted on https://ci.trustedfirmware.org/) */
@Field final boolean is_new_ci_env = !is_open_ci_env && (env.JENKINS_URL ==~ /\S+(trustedfirmware)\S+/)
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Oct 31, 2025

Choose a reason for hiding this comment

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

Please give the “new CI” a distinctive name. I don't want to change this file to start distinguishing between the “old new CI” and the “new new CI” in a few years.

vars/common.groovy Outdated
/* Indicates if CI is running on Open CI (hosted on https://ci.trustedfirmware.org/) */
@Field is_open_ci_env = env.JENKINS_URL ==~ /\S+(trustedfirmware)\S+/
/* Indicates if CI is running on Open CI (hosted on https://mbedtls.trustedfirmware.org/) */
@Field final boolean is_open_ci_env = env.JENKINS_URL ==~ /\S+(mbedtls\.trustedfirmware\.org)\S+/
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Oct 31, 2025

Choose a reason for hiding this comment

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

Non-blocker: I'd prefer something a little more structured and comprehensible, like

enum CI_Instance {
    CI_Other,           // Unexpected, may happen when trying out new infrastructure
    CI_ArmInternal,     // Arm internal CI (since 2017)
    CI_ArmOpenCI,       // Arm-maintained OpenCI (since 2025)
    CI_LinaroOpenCI,    // Linaro-maintained OpenCI (since 2022)
}

def CI_Instance ci_from_url(String url_string) {
    String host = url_string.toURL().getHost()
    if (host ==~ /\.arm\.com$/) {
        return CI_ArmInternal
    } else if (host ==~ /mbedtls.*\.trustedfirmware\.org$/) {
        return CI_LinaroOpenCI
    } else if (host ==~ /\.trustedfirmware\.org$/) {
        return CI_ArmOpenCI
    } else {
        return CI_Other
    }
}

@Field which_ci = ci_from_url(env.JENKINS_URL)

developer_guide.md

```
node (label) {
common.mbedtls_node (label) {
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Oct 31, 2025

Choose a reason for hiding this comment

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

More things need updating in the developer guide.

@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Nov 3, 2025
Open
13 tasks
@minosgalanakis
renamed CI's
87306f8 
- TF OpenCI -> TF OpenCI (legacy)
- New CI (testing) -> TF OpenCI

Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis
Updated the developer guide
320c37d 
Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis
Fixed a typo in ecr name for gen_jobs
87703e9 
Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis minosgalanakis force-pushed the dev/bensze01/openci-testing branch from 552c956 to 87703e9 Compare November 5, 2025 16:00
@minosgalanakis
Copy link
Contributor

minosgalanakis commented Nov 5, 2025 •
edited
Loading

As of 87703e9

Had to remove the code to implement this change. It was throwing me into a rabit hole of scripts permissions missalignment between CI's.

# When using toURL
Scripts not permitted to use staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods toURL java.lang.String. Administrators can decide whether to approve or reject this signature.

# When using the Java java.net.URL
Scripts not permitted to use method java.net.URL getHost. Administrators can decide whether to approve or reject this signature.

Testing jobs

gilles-peskine-arm
gilles-peskine-arm requested changes Nov 5, 2025
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me on code inspection. I can't think of anything else that would need updating in the guide.

Once the code changes are complete, please link the test jobs that you've run in the PR description.

developer_guide.md

#### Jenkins jobs

On OpenCI, the jobs are defined by YAML configuration files managed in a Gerrit instance: [browse code](https://review.trustedfirmware.org/plugins/gitiles/ci/mbedtls/mbed-tls-job-configs), [contributor setup](https://review.trustedfirmware.org/Documentation/user-upload.html), [reviews](https://review.trustedfirmware.org/q/project:ci/mbedtls/mbed-tls-job-configs+status:open). On the internal CI, the jobs and job configurations can be edited directly through the web interface.
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Nov 5, 2025

Choose a reason for hiding this comment

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

Can someone confirm whether the new OpenCI uses the same repository as the old OpenCI for the configuration?

I see a branch called openci-migration in that repository, is it relevant? Does it need to be merged as part of the CI transition?

Copy link
Contributor Author

@bensze01 bensze01 Nov 17, 2025

Choose a reason for hiding this comment

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

The "new" CI indeed uses the openci-migration branch for now (since we need slightly different configuration for the two CIs at the moment.)

It can be migrated back to the master branch at our leisure once the "old" OpenCI is shut down.

@minosgalanakis
Copy link
Contributor

minosgalanakis commented Nov 6, 2025

Head at 68c77aa

gilles-peskine-arm
gilles-peskine-arm previously approved these changes Nov 6, 2025
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Looks good to me at 68c77aa in terms of reading the code.

With respect to validation, given the changes here, I would like to see tests that exercise the following features: docker instantiation (so basically anything), actual use of arm-compilers because it gets special treatment (so e.g. a PR job or a release job), actual use of the various node labels, GitHub reporting, and a Dockerfile rebuild. So I propose, on all thre CIs:

mpg
mpg reviewed Nov 7, 2025
View reviewed changes
@mpg
Copy link
Contributor

mpg commented Nov 17, 2025

Thanks for fixing the link. However, I haven't done a full review, nor did I intend do. Are you expecting a full review from me, or just a check that my drive-by comment has been addressed?

gilles-peskine-arm
gilles-peskine-arm reviewed Nov 17, 2025
View reviewed changes
@minosgalanakis minosgalanakis force-pushed the dev/bensze01/openci-testing branch from 30738c9 to e5add1f Compare November 17, 2025 10:15
@minosgalanakis
Copy link
Contributor

minosgalanakis commented Nov 17, 2025 •
edited by gilles-peskine-arm
Loading

Testing at e5add1f

Internal CI

New CI Release Job

gilles-peskine-arm
gilles-peskine-arm approved these changes Nov 17, 2025
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@minosgalanakis minosgalanakis minosgalanakis left review comments

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@ronald-cron-arm ronald-cron-arm Awaiting requested review from ronald-cron-arm

@mpg mpg Awaiting requested review from mpg

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

enhancement New feature or request needs: review priority-high size-s Estimated task size: small (~2d)

Projects

Roadmap pull requests (new board)
Status: In Development

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Adapt Groovy code to the new CI

6 participants

@bensze01 @minosgalanakis @mpg @gilles-peskine-arm @dgreen-arm