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.

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

Create workflows to release Node.js adapter from own repo #521

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Create workflows to release Node.js adapter from own repo #521

wants to merge 1 commit into from
+146 −23

Conversation

mposolda
Copy link
Contributor

@mposolda mposolda commented Dec 10, 2024 •
edited
Loading

closes #520

Ability to release node.js adapter from it's own repository. PR introduces 2 workflows:

  • Release nightly - for trigger nightly releases of node.js adapter
  • Release - for trigger regular release of node.js adapter

Workflows are doing same steps like the related workflows in keycloak-rel. There is related PR to keycloak-rel, which removes the node.js related workflows from keycloak-rel : keycloak-rel-testing/keycloak-rel#45 (So far only PR to the "testing" fork of keycloak-rel)

Secrets needed

The workflows require secret present in the repository, which is not there yet:
NPM_TOKEN - Token needed to be able to publish to npm registry like https://www.npmjs.com/package/keycloak-connect/v/26.0.7 . Needed only for Release (not needed for Release nightly)

Note that GH_TOKEN or GITHUB_TOKEN is not needed as a secret in the repository due it is created automatically and has the needed permissions.

Testing

Workflows tested in my own fork (everything work as expected, but publishing to NPM was only executed with --dry-run, so did not publish anything. I don't have proper NPM_TOKEN anyway in my repository):
https://github.com/mposolda/keycloak-nodejs-connect/actions/runs/12242029832
https://github.com/mposolda/keycloak-nodejs-connect/actions/runs/12242053798

@mposolda mposolda requested review from jonkoops and stianst December 10, 2024 12:23
@mposolda mposolda mentioned this pull request Dec 10, 2024
Open
jonkoops
jonkoops requested changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@jonkoops jonkoops left a comment

Choose a reason for hiding this comment

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

Couple of notes:

  1. I think this workflow doesn't need individual jobs, this could all be handled in a single job entirely. This also reduces the amount of re-downloading artifacts and settig up tooling, and makes it all easier to follow.
  2. We can probably remove release related scripts such as set-version.sh and release.sh from the project root.

.github/workflows/x-release.yml Outdated

jobs:

show-inputs:
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for this, there is an expandable section in the action that shows all the inputs when ran on CI. See this job on the main repo for example:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1, copy/pasted it from the keycloak-rel, where it is used on many places. But agree not needed and will remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

.github/workflows/x-release.yml Show resolved Hide resolved
.github/workflows/x-release.yml Outdated Show resolved Hide resolved
.github/workflows/x-release.yml Outdated
git commit -a -m "Set version to ${{ inputs.tag }}"

- name: Tag commit
run: git tag --force ${{ inputs.tag }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we doing a --force here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, copy/paste from keycloak-rel :-)

I guess the --force is used for the case of broken release, which failed in some further step, but has already tags created. In that case, you are able to re-run the release workflow and eventually overwrite existing tag. But I guess we don't need for the node.js release as it is relatively simple IMO? So will try to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to remove --force

.github/workflows/x-release.yml
run: git tag --force ${{ inputs.tag }}

- name: Push changes
run: git push --force origin refs/tags/${{ inputs.tag }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This would only push the tag right? Don't we also want to push the commit itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The commit itself is pushed just into the tag. But it is not pushed to the main branch (or release/X.Y branch) itself. I think it is done intentionally that way and it is the approach used in all Keycloak repositories.

For example last commit in the 26.0.7 tag contains the version commit: https://github.com/keycloak/keycloak-nodejs-connect/commits/26.0.7/ .

But the release/26.0 branch itself, from which the tag was created, does not contain that version commit: https://github.com/keycloak/keycloak-nodejs-connect/commits/release/26.0 .

I prefer to be consistent with other Keycloak repositories and hence keep put the version commit just to the tag. Or did you mean something different?

.github/workflows/x-release.yml Outdated
else
echo 'false'
fi
)" >> $GITHUB_ENV
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not push this into the env, an individual step can have outputs that can be read in other steps (see GitHub documentation).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks. Was again copy/paste existing stuff from keycloak-rel . Will try to improve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

.github/workflows/x-release.yml Outdated Show resolved Hide resolved
.github/workflows/x-release.yml Outdated Show resolved Hide resolved
.github/workflows/x-release.yml Outdated Show resolved Hide resolved
@mposolda
Copy link
Contributor Author

mposolda commented Dec 13, 2024 •
edited
Loading

Couple of notes:

1. I think this workflow doesn't need individual jobs, this could all be handled in a single job entirely. This also reduces the amount of re-downloading artifacts and settig up tooling, and makes it all easier to follow.

2. We can probably remove release related scripts such as `set-version.sh` and `release.sh` from the project root.

+1 for the point 1. I will rewrite that to use the steps only and not individual jobs. Was trying to be consistent with keycloak-rel, but agree that this could be rather simplified since there is not much "options" needed as in keycloak-rel .

For (2), I will remove release.sh . For the set-version.sh, we should be careful as it is still used by keycloak-rel . Hence it can be removed after keycloak-rel/keycloak-rel#137 is merged and once we are sure that none of the keycloak-rel builds would be never triggered for the node.js branch from where we remove it. Probably better to remove it in dedicated PR to reduce the dependencies between various PRs? As currently this PR could be merged without the merge of keycloak-rel PR keycloak-rel/keycloak-rel#137 or vice-versa and hence they are not strictly dependent on each other.

@mposolda @jonkoops
Create workflows to release Node.js adapter from own repo
9e81c9f 
closes keycloak#520

Signed-off-by: mposolda <[email protected]>

Co-authored-by: Jon Koops <[email protected]>
Signed-off-by: Marek Posolda <[email protected]>
@mposolda mposolda requested a review from jonkoops December 16, 2024 10:09
@mposolda
Copy link
Contributor Author

@jonkoops Updated a PR to:

  • Address most of your inline comments and replied to some remaining
  • Using single job with individual steps
  • Removed a requirement of GH_SECRET as a secret in the repository and instead rely on the automatically created GITHUB_TOKEN, which is always available for GH actions (this was requested by @stianst ). Also updated the description of the PR to reflect that
  • Removed release.sh script, which is not needed

Re-review welcome

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

@stianst stianst Awaiting requested review from stianst

@jonkoops jonkoops Awaiting requested review from jonkoops

Requested changes must be addressed to merge this pull request.

Assignees

@jonkoops jonkoops

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create workflows to release Node.js adapter from own repo
2 participants
@mposolda @jonkoops