Add ability to deploy js adapter to maven repository. by pskopek · Pull Request #23425 · keycloak/keycloak

pskopek · 2023-09-21T10:11:50Z

This change will allow any downstream build to deploy keycloak-js-adapter tgz archive to maven repository by setting following properties:

deploy-js-to-maven-repo
maven.deploy.url
maven.repository.id

Change is set-version.sh script allows to use different version string for node package (as version string in maven and npm are not compatible).

jonkoops · 2023-09-21T10:31:24Z

Change is set-version.sh script allows to use different version string for node package (as version string in maven and npm are not compatible).

As mentioned under #23428 (comment), why not use semantic versioning everywhere? This would work both in NPM and Maven.

cypress · 2023-09-21T10:39:50Z

Passing run #9013 ↗︎

0	527	48	0	0

Details:

Merge `fc6fbdd` into `5070f41`...
Project: Keycloak Admin UI	Commit: `caef10e0ac ℹ️`
Status: Passed	Duration: 14:01 💡
Started: Sep 21, 2023 11:42 AM	Ended: Sep 21, 2023 11:56 AM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

pskopek · 2023-09-21T10:47:40Z

Change is set-version.sh script allows to use different version string for node package (as version string in maven and npm are not compatible).

As mentioned under #23428 (comment), why not use semantic versioning everywhere? This would work both in NPM and Maven.

The problem is that internal build system will change the version of artifacts. Then we are going to publish keycloak artifacts in public maven repository with the new version.
The same will be true for node packages (just different public repo). IMO we should have both versions close to each other and different from the upstream ones.

jonkoops · 2023-09-21T10:52:30Z

The problem is that internal build system will change the version of artifacts. Then we are going to publish keycloak artifacts in public maven repository with the new version.

That is still possible with semantic versioning, no? Could you elaborate on what makes those versions different than the ones we use for Keycloak upstream?

jonkoops · 2023-09-21T11:13:28Z

Just discussed this with @pskopek and I believe we can create valid semantic versions for RHBK by using the following format 22.0.2+redhat-00001. Everything after the + is a build identifier.

Closes keycloak#23312 Signed-off-by: Peter Skopek <pskopek@redhat.com>

jonkoops · 2023-09-21T11:20:49Z

 #!/bin/bash -e

 NEW_VERSION=$1
+NEW_NPM_VERSION=${2:-${1}}


I still think we should not be using a seperate version for NPM. Instead the input versions should all adhere to valid semantic versions.

This line has nothing to do with using valid semantic versions. It allows our downstream build script to override the NPM version if needed (mainly for debug/investigation purpose).
Even if one uses set-version.sh with one parameter and specifies "non-semantic" version it will use it anyway (no validation of version is present).
Our production build script is using "./set-version.sh use_current".

Why would it need to do that? Wouldn't you just override all the versions?

The internal build system assigns version to keycloak (and aligns dependencies) before start of maven build.
We have to use what was assigned. The problem is that assigned version is Maven compatible, but not npm compatible.
Therefore we need two of them. See my comment in set-version.sh script.

Therefore we need two of them. See my comment in set-version.sh script.

Or the version can be a valid semantic version, in which case it would work for both.

ghost · 2023-09-21T11:27:50Z

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.ui.account2.DeviceActivityTest#timeLocaleTest

Keycloak CI - Account Console IT (firefox)

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a org.keycloak.testsuite.util.URLAssert URL expected to begin with: https://localhost:8543/auth/realms/test/protocol/openid-connect/auth ; actual URL: https://localhost:8543/auth/realms/test/account/#/security/device-activity within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:985)
...

Report flaky test

org.keycloak.testsuite.javascript.JavascriptAdapterTest#testSilentCheckSso

Keycloak CI - Base IT (5)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

ghost

Unreported flaky test detected, please review

ghost

Unreported flaky test detected, please review

ghost · 2023-09-21T13:57:35Z

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.ui.account2.WelcomeScreenTest#accountSecurityTest

Keycloak CI - Account Console IT (firefox)

java.lang.AssertionError: Expected DeviceActivityPage but was Keycloak Account Management (https://localhost:8543/auth/realms/test/account/#/security/device-activity&state=5d176f17-f483-48d5-8ec2-9f158a234341&session_state=06f27b59-7156-49bf-8ee5-c500dc8258dd&iss=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Ftest&code=0d888e0c-1440-422b-a31a-ef84053fdd5b.06f27b59-7156-49bf-8ee5-c500dc8258dd.5fb9324b-bc2d-45b3-b599-9278238cc737)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.page.AbstractPage.assertCurrent(AbstractPage.java:110)
	at jdk.internal.reflect.GeneratedMethodAccessor110.invoke(Unknown Source)
...

Report flaky test

stianst · 2023-09-22T08:31:33Z

            </plugin>
        </plugins>
    </build>
+    <profiles>


This looks unnecessarily complex, and adds some custom Maven registry settings just for this module. Why not just attach the tgz with:

<plugin> <groupId>org.codehaus.mojo</groupId> <artifactId>build-helper-maven-plugin</artifactId> <executions> <execution> <id>attach-artifacts</id> <phase>package</phase> <goals> <goal>attach-artifact</goal> </goals> <configuration> <artifacts> <artifact>${project.basedir}/assembly.xml <file>target/keycloak-js-${project.version}.tgz</file> <type>tar.gz</type> </artifact> </artifacts> </configuration> </execution> </executions> </plugin>

and change the configuration for maven-deploy-plugin to `true. That way it's just uploaded along the other artifacts.

I can certainly do this. I just didn't want to alter original maven build behavior.

stianst · 2023-09-22T08:33:00Z

+if [ $NEW_VERSION == 'use_current' ] ; then
+  # obtain NEW_VERSION from maven pom file
+  # - useful for downstream projects which already set the version in pom files
+  NEW_VERSION=$(./get-version.sh)
+  NEW_NPM_VERSION=$(echo $NEW_VERSION | awk -F '.' '{ print $1"."$2"."$3"+"$4 }')
+else


What's the purpose of this?

Follow up from #23425 (comment).
This will assign NPM compatible version string. The third dot is changed to '+' as suggested by @jonkoops .

stianst

I'm not really following the need to have different versions for NPMs, and changes to the set-version script. Both Maven and NPM require a semantic version, and isn't the versions used by PNC/RH stuff already semantic compatible and can be used as is?

pskopek · 2023-09-22T13:11:23Z

I'm not really following the need to have different versions for NPMs, and changes to the set-version script. Both Maven and NPM require a semantic version, and isn't the versions used by PNC/RH stuff already semantic compatible and can be used as is?

RH version is not semantic compatible. https://semver.org/#backusnaur-form-grammar-for-valid-semver-versions
22.0.3 -> 22.0.3.redhat-00001.
Maven can use RH version, but NPM cannot.

jonkoops · 2023-09-22T14:13:52Z

Maven can use RH version, but NPM cannot.

Then let's change the version so it works for both, no? What is preventing us from doing that?

pskopek · 2023-09-22T14:29:00Z

Maven can use RH version, but NPM cannot.

Then let's change the version so it works for both, no? What is preventing us from doing that?

We cannot change the suffix generated by internal build system. There are so many other parts that use the knowledge of this format.

jonkoops · 2023-09-25T08:59:04Z

If we need this for the release of 22, I suggest we only land this patch for the release branch. I'd rather not have this code end up in main just to handle build-specific things for RHBK.

We cannot change the suffix generated by internal build system.

Why is this not possible exactly?

stianst · 2023-09-25T12:44:41Z

There's nothing we can do about the versions used downstream, which is a shame since they are not even semver compatible.

I've created a simpler proposal (at least IMO) for this here: #23512

edewit · 2023-09-26T12:07:07Z

an easier way to have keycloak-js in maven central is by using mvnpm it can pull any npm.org package and push it into maven central. It works as a proxy repository have a look at it https://mvnpm.org/about

<dependency>
	<groupId>org.mvnpm</groupId>
	<artifactId>keycloak-js</artifactId>
	<version>22.0.3</version>
	<scope>runtime</scope>
</dependency>

pskopek · 2023-09-26T13:04:26Z

Closing in favor of back port of this PR #23512

pskopek requested review from a team as code owners September 21, 2023 10:11

ghost added the team/ui label Sep 21, 2023

pskopek mentioned this pull request Sep 21, 2023

Add ability to deploy js adapter to maven repository. #23428

Closed

stianst requested review from jonkoops and stianst September 21, 2023 11:11

Add ability to deploy js adapter to maven repository.

fc6fbdd

Closes keycloak#23312 Signed-off-by: Peter Skopek <pskopek@redhat.com>

pskopek force-pushed the deploy-js-adapter-to-mvn branch from bddbc2f to fc6fbdd Compare September 21, 2023 11:19

jonkoops suggested changes Sep 21, 2023

View reviewed changes

ghost added the flaky-test label Sep 21, 2023

This was referenced Sep 21, 2023

Flaky test: org.keycloak.testsuite.ui.account2.UpdateEmailTest#updateEmailLinkVisibleWithUpdateEmailActionEnabled #23336

Closed

Flaky test: org.keycloak.testsuite.forms.BrowserFlowTest#testAlternativeNonInteractiveExecutorInSubflow #22644

Closed

ghost reviewed Sep 21, 2023

View reviewed changes

This was referenced Sep 21, 2023

Flaky test: org.keycloak.testsuite.ui.account2.ApplicationsTest#toggleApplicationDetailsTest #23362

Closed

Flaky test: org.keycloak.testsuite.ui.account2.ApplicationsTest#applicationListTest #22169

Closed

ghost reviewed Sep 21, 2023

View reviewed changes

stianst reviewed Sep 22, 2023

View reviewed changes

pskopek closed this Sep 26, 2023

Uh oh!

Conversation

pskopek commented Sep 21, 2023

Uh oh!

jonkoops commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cypress Bot commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Passing run #9013 ↗︎

Uh oh!

pskopek commented Sep 21, 2023

Uh oh!

jonkoops commented Sep 21, 2023

Uh oh!

jonkoops commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 21, 2023

Unreported flaky test detected

org.keycloak.testsuite.ui.account2.DeviceActivityTest#timeLocaleTest

org.keycloak.testsuite.javascript.JavascriptAdapterTest#testSilentCheckSso

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 21, 2023

Unreported flaky test detected

org.keycloak.testsuite.ui.account2.WelcomeScreenTest#accountSecurityTest

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stianst left a comment

Choose a reason for hiding this comment

Uh oh!

pskopek commented Sep 22, 2023

Uh oh!

jonkoops commented Sep 22, 2023

Uh oh!

pskopek commented Sep 22, 2023

Uh oh!

jonkoops commented Sep 25, 2023

Uh oh!

stianst commented Sep 25, 2023

Uh oh!

edewit commented Sep 26, 2023

Uh oh!

pskopek commented Sep 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonkoops commented Sep 21, 2023 •

edited

Loading

cypress Bot commented Sep 21, 2023 •

edited

Loading

jonkoops commented Sep 21, 2023 •

edited

Loading