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

feat: new rafiki release pipeline and nodejs bump to v20 #2510

Merged
merged 23 commits into from
Apr 3, 2024
Merged

feat: new rafiki release pipeline and nodejs bump to v20 #2510

merged 23 commits into from
Apr 3, 2024

Conversation

golobitch
Copy link
Collaborator

Changes proposed in this pull request

  • Update NodeJS version from v18 to v20
    • v18.x is Maintenance LTS and it will end on 2025-04-30
    • v20.x is Active LTS
  • New Rafiki release pipeline
    • Do everything that now has been done
    • Run docker image scan for any known vulnerabilities and break build if severity is HIGH or CRITICAL
    • Publish docker image to GitHub repository
    • Create tag and release in GitHub

Context

Rafiki release pipeline is now designed that is possible to do quick deploys.

Branching strategy

For every major / minor version change, you need to create new release branch, but not for patch change. If you want to create release v2.1.0, you would need to create following release branch: release/v2.1.0. When there will be push to this repository, patch number will increment. Ideally, when release branch is created, there should be feature freeze, so that we do not need to update minor version. Instead, in release branch, we should push (after creation), only bugfixes, to make this release stable.

Following release branches are supported:

release/v1.0.0
release/v1.0.0-beta
release/v1.0.0-alpha

Also, correct corresponding tags and releases will be created in GitHub.

On branch release/v1.0.0-beta, first tag and release would be v1.0.0-beta and second one v1.0.1-beta

Docker build, scan, publish and release creation will be triggered only on release branches.

For the documentation on branching strategy, tags, etc, I will create new PR with updated documentation.

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Bruno collection updated

@github-actions github-actions bot added type: ci Changes to the CI pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. pkg: auth Changes in the GNAP auth package. pkg: mock-ase type: documentation (archived) Improvements or additions to documentation labels Mar 7, 2024
Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for brilliant-pasca-3e80ec failed.

Name Link
🔨 Latest commit aaf9d49
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/660d2f57828c9c0008dfa2b2

.github/workflows/pr_title_check.yml Outdated Show resolved Hide resolved
.github/workflows/node-build.yml Outdated Show resolved Hide resolved
.github/workflows/node-build.yml Show resolved Hide resolved
uses: actions/checkout@v4
- name: Generate CHANGELOG data
id: changelog
uses: requarks/changelog-action@v1
Copy link
Member

Choose a reason for hiding this comment

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

This creates the changelog based on the template, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This actually generates changelog data based on commits between current and previous same tag. It does not create changelog. If you want, I can also generate CHANGELOG.md here or append the data to it?

Copy link
Member

Choose a reason for hiding this comment

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

Do you know how it orders the commits in the generated changelog?
Right now, we have the following defined: https://github.com/interledger/rafiki/blob/main/.github/release.yaml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, if commit has note BREAKING CHANGE then this breaking changes will be always on top. After that, there will be bug fixes, features, chores, etc. I just need to check for dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sabineschaller I'm not sure if this action takes into account the label of the PRs and the breaking > general changes > dependency updates order, since it checks commits and not PRs.

That being said, it does look like though we should get into a habit of doing feat!: ... (or adding BREAKING CHANGE to the to-be squashed commit), so we could see & auto generate breaking changes just from the commits

Copy link
Member

Choose a reason for hiding this comment

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

@mkurapov I think it needs to be in the to-be-squashed commit either way, so we need to start adding the !. And we can try training people to add the BREAKING CHANGE footer.

@sabineschaller
Copy link
Member

sabineschaller commented Mar 8, 2024

Also, I keep forgetting to increase the version in the package.json. I was thinking about just removing it

but the frontend displays that version so we may have to keep it.
Long story short, can we add a check in the workflow that checks whether the version has been bumped on the release branch?

@golobitch
Copy link
Collaborator Author

Also, I keep forgetting to increase the version in the package.json. I was thinking about just removing it

but the frontend displays that version so we may have to keep it. Long story short, can we add a check in the workflow that checks whether the version has been bumped on the release branch?

We can maybe just update the package.json when we are creating changelog data? But that would be only on the release branch.

Action can create PR into main branch with version update in package.json. Should we go with that?

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@sabineschaller
Copy link
Member

We can maybe just update the package.json when we are creating changelog data? But that would be only on the release branch.

Never mind, the team decided to remove the version.

@@ -35,9 +35,9 @@ Never heard of Interledger before? Or would you like to learn more? Here are som

Copy link
Contributor

Choose a reason for hiding this comment

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

While we are here, in the "Local Development Environment" section above, we can remove the nvm references and just say that the prerequisites are pnpm & node v20.

cc @sabineschaller since it looks like you edited the file

Copy link
Member

Choose a reason for hiding this comment

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

Sure

.github/workflows/node-build.yml Outdated Show resolved Hide resolved
severity-cutoff: high
output-format: table

docker-trivy:
Copy link
Contributor

Choose a reason for hiding this comment

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

grype and trivy: how much do they overlap? ie is it good to keep both vs just picking one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, the difference is basically in the data sources that the scanner is using. Trivy is using more data sorces than grype, however, grype is using some of the data sources that trivy is not. So basically, we can keep both, or at least, just Trivy. Your thought?

Copy link
Contributor

Choose a reason for hiding this comment

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

might as well have a broader check, we can keep both - just wanted to understand the differences

uses: actions/checkout@v4
- name: Generate CHANGELOG data
id: changelog
uses: requarks/changelog-action@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

@sabineschaller I'm not sure if this action takes into account the label of the PRs and the breaking > general changes > dependency updates order, since it checks commits and not PRs.

That being said, it does look like though we should get into a habit of doing feat!: ... (or adding BREAKING CHANGE to the to-be squashed commit), so we could see & auto generate breaking changes just from the commits

.github/workflows/node-build.yml Outdated Show resolved Hide resolved
.github/workflows/node-build.yml Outdated Show resolved Hide resolved
uses: docker/build-push-action@v5
with:
push: false
platforms: linux/amd64,linux/arm64
Copy link
Member

Choose a reason for hiding this comment

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

Can we only build on both platforms if we actually release? Otherwise, this will take forever.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can. So basically, build only for linux/amd64 on feature branches and both platforms for release branch?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that is what we used to do.

@mkurapov
Copy link
Contributor

BTW I changed the Netlify NODE_VERSION back to 18 (for the rafiki.dev documentation) for now since we have some doc changes in PRs we wanted previews on. So once this is merged in, it can be set to 20.

.github/workflows/node-build.yml Outdated Show resolved Hide resolved
.github/workflows/node-build.yml Outdated Show resolved Hide resolved
.github/workflows/node-build.yml Outdated Show resolved Hide resolved
severity-cutoff: high
output-format: table

docker-trivy:
Copy link
Contributor

Choose a reason for hiding this comment

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

might as well have a broader check, we can keep both - just wanted to understand the differences

@golobitch golobitch self-assigned this Mar 23, 2024
@golobitch
Copy link
Collaborator Author

@sabineschaller @mkurapov we can do another interation of a review now.

mkurapov
mkurapov previously approved these changes Mar 27, 2024
Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

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

👍 good to go after main's merged in

@sabineschaller
Copy link
Member

I'll re-review as soon as main is merged in :-)

@golobitch
Copy link
Collaborator Author

@mkurapov @sabineschaller rebased to main & updated PR

@golobitch golobitch requested a review from mkurapov March 29, 2024 19:34
mkurapov
mkurapov previously approved these changes Mar 29, 2024
@golobitch
Copy link
Collaborator Author

@mkurapov @sabineschaller I am not authorized to merge this PR.

@mkurapov
Copy link
Contributor

mkurapov commented Apr 2, 2024

@mkurapov @sabineschaller I am not authorized to merge this PR.

I'll give you access... ✅

@golobitch
Copy link
Collaborator Author

Netlify builds will fail due to node bump. After we merge this PR into master, we should also bump Netlify

@@ -134,6 +134,29 @@ jobs:
- name: Perform CodeQL Analysis
uses: github/codeql-action/analyze@v3

integration-test:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add this to the needs list for the node-build -> we should probably not run node-build if we fail integration tests.

CC @BlairCurrey

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I'd say so too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

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

🚀

@mkurapov
Copy link
Contributor

mkurapov commented Apr 3, 2024

@tadejgolobic We have a branch protection rule set up for all_pr_checks_passed. Since we don't have that anymore, I can change that so that it should be node-build instead.

Maybe also docker-build ?

@mkurapov
Copy link
Contributor

mkurapov commented Apr 3, 2024

@golobitch done, looks like its mergeable

@tadejgolobic
Copy link
Contributor

@tadejgolobic We have a branch protection rule set up for all_pr_checks_passed. Since we don't have that anymore, I can change that so that it should be node-build instead.

Maybe also docker-build ?

Should I just create one more job whose name it will be all_pr_checks_passed ? That would probably be the easiest solution

@mkurapov
Copy link
Contributor

mkurapov commented Apr 3, 2024

@tadejgolobic I don't mind just adding docker-build as a separate required check in the settings, it's straightforward enough and doesn't make a big difference.

tl;dr we can merge IMO

@golobitch golobitch merged commit 021a7fa into interledger:main Apr 3, 2024
26 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: auth Changes in the GNAP auth package. pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. pkg: mock-ase type: ci Changes to the CI type: documentation (archived) Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants