KEYCLOAK-14547: Make New Account Console the default. #7267

ssilvert · 2020-07-15T00:43:41Z

This PR makes the following changes:

New Account Console enabled by default
Default themes are now "keycloak.v2" and "rh-sso.v2"
Original "keycloak" is theme still available
~~Account REST API enabled by default~~ (done in another PR)
You can still disable New Account Console and Account REST API if you want
~~At build time, the default theme is driven by new property "product.default-theme"~~
Old tests need to still assume old account console. So AbstractKeycloak test disables new account console. If you want a test to use new account console you can use @EnableFeature(value=Profile.Feature.ACCOUNT2). Or you can extend from MainAbstractKeycloakTest instead of AbstractKeycloakTest.

@ssilvert adding the test fixes that @vmuzikar implemented here, so we don't miss it:

Depends on: Test fixes ssilvert/keycloak#10

vmuzikar · 2020-07-20T17:26:47Z

@ssilvert I'll review tomorrow (Wed at the latest).

Since this is quite a huge change testsuite-wise, please make sure to run both test pipelines (all tests included, not just UI) and for both KC and RH-SSO (some tests might be relying on some theme name) if you haven't already. Thank you.

ssilvert · 2020-07-20T19:52:24Z

@ssilvert I'll review tomorrow (Wed at the latest).

Since this is quite a huge change testsuite-wise, please make sure to run both test pipelines (all tests included, not just UI) and for both KC and RH-SSO (some tests might be relying on some theme name) if you haven't already. Thank you.

Admin console tests passed (number 968)
New account console tests passed (number 188)
Travis passed last time I checked.

I'm not sure how I should run the others you mentioned?

vmuzikar

@ssilvert I've sent you an email with instructions for the next time and triggered the pipelines myself. ;)
https://keycloak-jenkins.com/job/universal-test-pipeline-server/1983/
https://keycloak-jenkins.com/job/universal-test-pipeline-adapters/1584/

Added a few comments. We also have to wait for the pipelines results.

...te/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/ProfileAssume.java

vmuzikar · 2020-07-21T10:17:06Z

...gration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AbstractKeycloakTest.java

 */
-@RunWith(KcArquillian.class)
-@RunAsClient
-public abstract class AbstractKeycloakTest {


I have to admit I don't like the approach with renaming and creating new abstract classes. The naming is quite confusing – AbstractKeycloakTest vs MainAbstractKeycloakTest. But more importantly, it breaks the class hierarchy which results in duplicate code e.g. in AbstractUiTest.

The way I see it we have two options.

Move the abstract class that disables the new console much, much lower in the hierarchy. So that the abstract classes could be used by the new console tests and code duplication wouldn't be necessary.

Disable the new console in the first abstract class and then re-enable it only in the tests that need it. The number of tests for the new console is really just a fraction of what we have, so maybe that would be a cleaner approach.

In any case, this change should be as small as possible because it's only temporary. Sooner or later the old console will be deprecated and we'll have to fix all the tests that rely on it.

I have to admit I don't like the approach with renaming and creating new abstract classes. The naming is quite confusing – AbstractKeycloakTest vs MainAbstractKeycloakTest. But more importantly, it breaks the class hierarchy which results in duplicate code e.g. in AbstractUiTest.

The way I see it we have two options.

Move the abstract class that disables the new console much, much lower in the hierarchy. So that the abstract classes could be used by the new console tests and code duplication wouldn't be necessary.

Disable the new console in the first abstract class and then re-enable it only in the tests that need it. The number of tests for the new console is really just a fraction of what we have, so maybe that would be a cleaner approach.

In any case, this change should be as small as possible because it's only temporary. Sooner or later the old console will be deprecated and we'll have to fix all the tests that rely on it.

I'm not really happy with this either.

I spent a fair amount of time trying to figure out what would be the least disruptive to the current code base.

I don't think your first suggestion would work at all without changing a lot of tests. If I recall, a lot of tests extend AbstractKeycloakTest directly.

I wouldn't want to go through all the tests that need new account console and enable NAC individually. That just seems wrong since we want to use NAC going forward.

IMO, the only thing that made the code a little more ugly was that I had to copy some code from AbstractAuthorizationTest to AbstractUITest. Because of the way the hierarchy is set up there was no good way to get around it without figuring out exactly what each test was using from the base class.

This is what happens when a code base relies so heavily on inheritance instead of delegation.

That being said, the whole thing could definitely use some refactoring but it's just a matter of deciding how much time you want to spend.

I don't think your first suggestion would work at all without changing a lot of tests. If I recall, a lot of tests extend AbstractKeycloakTest directly.

Ah, yes, you're correct. Option 1 won't work.

I wouldn't want to go through all the tests that need new account console and enable NAC individually. That just seems wrong since we want to use NAC going forward.

Actually, I think you wouldn't need to go trough all the tests that need NAC. The annotations to enable it are already there, the only change we'd have to do now is to disable NAC in the most abstract class. Once we fixed the tests to work with NAC, we'll just remove those annotations. That seems to me like the least invasive option. If it works, of course. ;)

That being said, the whole thing could definitely use some refactoring but it's just a matter of deciding how much time you want to spend.

+1000
That being said, it's ugly but we don't need to make it even more ugly now. :)

Actually, I think you wouldn't need to go trough all the tests that need NAC. The annotations to enable it are already there, the only change we'd have to do now is to disable NAC in the most abstract class. Once we fixed the tests to work with NAC, we'll just remove those annotations. That seems to me like the least invasive option. If it works, of course. ;)

I tried that and it didn't appear to work. Even if it did things would get confusing. You end up with an inheritance hierarchy where a base class has @EnableFeature and a subclass has @DisableFeature. Or vice versa. So you don't know who wins.

I did find that using the annotation on a method takes precedence, over having it at the class level. So that helped out a bit.

IMHO there's no problem with a parent class disabling feature and some child re-enabling it. I'm sure we could fix the implementation if it doesn't work like that.

As an alternative, we could globally disable NAS on testsuite level by some other way (without using the annotation) and use the annotation just for enabling where needed. It's all just temporary anyway so we should do as little changes in the class hierarchy as possible.

IMHO there's no problem with a parent class disabling feature and some child re-enabling it. I'm sure we could fix the implementation if it doesn't work like that.

I think it is a problem because there is no hard and fast rule about who should win. And even if you know how to determine the winner you must inspect the entire hierarchy to find all the @enable and @disable annotations. That's very confusing for the developer.

As an alternative, we could globally disable NAS on testsuite level by some other way (without using the annotation) and use the annotation just for enabling where needed. It's all just temporary anyway so we should do as little changes in the class hierarchy as possible.

We both agree that things have gotten messy. Let's fix it in another JIRA.

I think it is a problem because there is no hard and fast rule about who should win. And even if you know how to determine the winner you must inspect the entire hierarchy to find all the @enable and @disable annotations. That's very confusing for the developer.

It already works like that – the entire hierarchy is transparently checked for the annotations thanks to @Inherited. ;) And to clearly determine who wins is just a matter of updating checkAnnotatedElementForFeatureAnnotations. I still believe this is a cleaner way than to break the class hierarchy.

We both agree that things have gotten messy. Let's fix it in another JIRA.

Sure, a follow-up JIRA can fix things but IMHO we don't need to make it even more messy in this PR. :)

@ssilvert PTAL at ssilvert#8. It makes the enabling/disabling features more predictable so that we can disable NAC in abstract test class and the re-enable it in NAC tests.

@mhajas Could you please check ssilvert@82fd457 if it makes sense to you? It touches your KeycloakContainerFeaturesController.

@vmuzikar It makes sense to me. However, could you please run the whole testsuite with these changes? There are quite a lot of tests using it, so we should make sure this doesn't break something.

@mhajas Sure, I plan to do that once @ssilvert approves the changes as well. Thank you for checking it!

...n/tests/other/base-ui/src/test/java/org/keycloak/testsuite/ui/account2/ApplicationsTest.java

vmuzikar · 2020-07-21T10:41:20Z

services/src/main/java/org/keycloak/theme/DefaultThemeManager.java

@@ -72,7 +73,7 @@ public Theme getTheme(String name, Theme.Type type) {
        if (theme == null) {
            theme = loadTheme(name, type);
            if (theme == null) {
-                theme = loadTheme("keycloak", type);
+                theme = loadTheme("keycloak.v2", type);


I'm not sure if it isn't confusing that all themes (even the old ones) use v2 with the old keycloak still available. The problem I see is that keycloak and keycloak.v2 are technically same themes (except account console) but the v2 could make the impression something's new in all themes.

But I'm ok with this if @stianst is.

This line of code is only used when you haven't selected any theme. It shows up in the UI as "Select one".

You do see both keycloak and keycloak.v2 in the dropdown when you go to select a theme. But that actually turns out kind of nice because there is some symmetry. And without it, the code above wouldn't work. You would need special logic to know that if it's a certain type then you have to fall back to "keycloak".

Yes, the line I selected here is not very accurate. My comment above is more generic – if it's ok to have identical keycloak and keycloak.v2. I'll wait for @stianst decision here.

vmuzikar · 2020-08-05T14:10:16Z

@ssilvert I'm afraid I've got some bad news. It seems that auth-server-remote is not supposed to work with the enable/disable feature annotations. All tests that currently use that annotations are excluded when auth-server-remote is used. That means we can't put @DisableFeature at any abstract class as it would break the remote testsuite. @vramik and @mhajas know most about this but both are on PTO at the moment. I suggest we wait for their input (unless we find a completely other way to do this).

vmuzikar · 2020-08-05T14:36:34Z

@ssilvert Scratch that. I think I know what's going on in there. I'll be able create a workaround but it'll require an extra step before running the tests, i.e. update to the how-to-run instructions as well as test pipeline.

vmuzikar

@ssilvert Unfortunately, I've found more issue we need to address before merge.

1. Broken CrossDC and Cluster tests

There's the same issue as with the remote test. Since we need to use @DisableFeature in an abstract class, we need to have admin user created before running any tests. This is because enabling/disabling feature is just an admin REST API call which of course fails when there's no admin user created. Normally, admin user is created by keycloak-add-user.json (WF/EAP) and programatically in KeycloakOnUndertow (Undertow) respectively. There's a failsafe in AbstractKeycloakTest but it doesn't work now because we need an admin user even before this is executed.

In remote tests, I've solved it by creating the admin user manually but I've got no clue how to do this for CrossDC and Cluster tests. Maybe @mposolda could give you some hints (once he's returned from his PTO)?

As an alternative, we could try to move the failsafe from abstract test to some Arquillian service to make sure it's executed before enabling/disabling features. This would also remove the need for the extra step when running remote tests. But I'm not sure if this wouldn't introduce some new instabilities. In the service, we couldn't probably use Drone or any of our tweaks for WebDriver. But you can try it if you want. :)

2. Node.js tests are broken

Some of them rely on the account console. The fix should be fairly simple – just disabling NAC. This will have to be done both in the repo and in the pipeline. Could you please fix that, @ssilvert?

abstractj

@ssilvert as we discussed today, we need to update the version for the new Account REST API from v1 to v1alpha1

The following files need to change:

keycloak/common/src/main/java/org/keycloak/common/enums/AccountRestApiVersion.java

Lines 28 to 30 in c27571b

V1("v1");

public static final AccountRestApiVersion DEFAULT = V1;

Please let me know if you would like me to submit a PR and I can do that.

ssilvert · 2020-08-17T18:53:31Z

@ssilvert as we discussed today, we need to update the version for the new Account REST API from v1 to v1alpha1

The following files need to change:

keycloak/common/src/main/java/org/keycloak/common/enums/AccountRestApiVersion.java

Lines 28 to 30 in c27571b

V1("v1");

public static final AccountRestApiVersion DEFAULT = V1;

Please let me know if you would like me to submit a PR and I can do that.

I'll do it.

ssilvert · 2020-08-17T19:02:15Z

@ssilvert Unfortunately, I've found more issue we need to address before merge.

1. Broken CrossDC and Cluster tests

There's the same issue as with the remote test. Since we need to use @DisableFeature in an abstract class, we need to have admin user created before running any tests. This is because enabling/disabling feature is just an admin REST API call which of course fails when there's no admin user created. Normally, admin user is created by keycloak-add-user.json (WF/EAP) and programatically in KeycloakOnUndertow (Undertow) respectively. There's a failsafe in AbstractKeycloakTest but it doesn't work now because we need an admin user even before this is executed.

In remote tests, I've solved it by creating the admin user manually but I've got no clue how to do this for CrossDC and Cluster tests. Maybe @mposolda could give you some hints (once he's returned from his PTO)?

As an alternative, we could try to move the failsafe from abstract test to some Arquillian service to make sure it's executed before enabling/disabling features. This would also remove the need for the extra step when running remote tests. But I'm not sure if this wouldn't introduce some new instabilities. In the service, we couldn't probably use Drone or any of our tweaks for WebDriver. But you can try it if you want. :)

2. Node.js tests are broken

Some of them rely on the account console. The fix should be fairly simple – just disabling NAC. This will have to be done both in the repo and in the pipeline. Could you please fix that, @ssilvert?

@vmuzikar Where are we on this stuff? Does #301 fix some or all of it?

vmuzikar · 2020-08-20T06:42:30Z

@vmuzikar Where are we on this stuff? Does #301 fix some or all of it?

@ssilvert If you mean keycloak/keycloak-pipeline#301, then I'm afraid no. It fixes neither of the issues – it's related only to remote tests, not CrossDS or Node.js.

vmuzikar · 2020-08-20T08:13:52Z

@ssilvert I just noticed, changes from ssilvert#8 (which were merged) are now missing from this PR. Could you please check it? Thank you.

ssilvert · 2020-08-20T12:55:51Z

@ssilvert I just noticed, changes from ssilvert#8 (which were merged) are now missing from this PR. Could you please check it? Thank you.

Bet I forgot to pull your changes down locally. Then when I made the change Bruno wanted, the force-push wiped out your changes. Glad you found that. I'll fix it.

ssilvert · 2020-08-20T19:48:30Z

@ssilvert I just noticed, changes from ssilvert#8 (which were merged) are now missing from this PR. Could you please check it? Thank you.

@vmuzikar Your branch with the changes has been removed. Can you restore the branch and resubmit the PR?

vmuzikar · 2020-08-20T20:00:07Z

@ssilvert Done. Sorry about that.

vmuzikar

@ssilvert Thanks for the update! I've added a few minor comments.

Please note I was NOT able to manually test the RH-SSO dist due to some missing artifacts which is unrelated to this PR. I've got PTO on Monday and public holiday on Tuesday, so if you are going to merge this when I'm gone, please double check RH-SSO works correctly.

Also, I've fixed hopefully all the tests: ssilvert#10. However, I tested my changes before today's rebase, so we should run both pipelines again once you've merged and squashed my PR. Also created KEYCLOAK-16228 to track follow-up changes.

vmuzikar · 2020-11-13T15:53:59Z

common/src/main/java/org/keycloak/common/Version.java

@@ -35,6 +35,7 @@
    public static String RESOURCES_VERSION;
    public static String BUILD_TIME;
    public static String DEFAULT_PROFILE;
+    public static String DEFAULT_THEME;


This doesn't seem to be used anywhere.

Yes, missed that one.

This doesn't seem to be used anywhere.

Fixed

vmuzikar · 2020-11-13T16:00:54Z

services/src/main/java/org/keycloak/theme/DefaultThemeManager.java

+    private String typeBasedDefault(Theme.Type type) {
+        if ((type == Theme.Type.ACCOUNT) && isAccount2Enabled) {
+            return "keycloak.v2";
+        }
+
+        return "keycloak";
+    }


I think we shouldn't hardcode this here. Cannot we have some property for this is Theme.Type, perhaps?

That's the way this has always worked. It's kind of a super-fallback.

vmuzikar · 2020-11-13T16:01:31Z

services/src/main/java/org/keycloak/theme/DefaultThemeManager.java

+        if (!isAccount2Enabled && theme.getName().equals("keycloak.v2")) {
+            theme = loadTheme("keycloak", type);
+        }
+
+        if (!isAccount2Enabled && theme.getName().equals("rhsso.v2")) {
+            theme = loadTheme("rhsso", type);
+        }


I haven't deeply investigated this, but it seems to me a bit suspicious. Why is it even necessary? Don't we set the correct theme here?

keycloak/services/src/main/java/org/keycloak/theme/DefaultThemeManager.java

Line 85 in 734ffb8

theme = loadTheme(typeBasedDefault(type), type);

This is needed in the case where you disable the new account console. The line you cite is rarely executed.

vmuzikar · 2020-11-13T16:17:40Z

services/src/main/java/org/keycloak/theme/DefaultThemeSelectorProvider.java

+            if ((type == Theme.Type.ACCOUNT) && isAccount2Enabled) {
+                name = name.concat(".v2");
+            }


Again, I'm not sure this is the right place to hardcode this. What if we have more v2 themes in the future? Cannot we have some property for this is Theme.Type, perhaps?

Again, I'm not sure this is the right place to hardcode this. What if we have more v2 themes in the future? Cannot we have some property for this is Theme.Type, perhaps?

This is what Stian wanted instead of the way I coded it before (which did use a property).

Ok. Even though I'm a bit concerned about that we have the "v2" logic spread at many places (and therefore it won't be quite straightforward to e.g. add v2 to other themes, or v3 to this theme), I'm ok with it since @stianst is.

vmuzikar · 2020-11-13T16:54:43Z

services/src/main/java/org/keycloak/theme/DefaultThemeManager.java

+    private String typeBasedDefault(Theme.Type type) {
+        if ((type == Theme.Type.ACCOUNT) && isAccount2Enabled) {
+            return "keycloak.v2";
+        }
+
+        return "keycloak";
+    }


IMHO this logic deserves a test.

This code only executes when a theme is set for the realm but it can't find a ThemeProvider. I'm not even sure how to make that happen, so I'm guessing it would be a bear to write a test. We have never had a test for that and it's really not that important whether it returns "keycloak.v2" or "keycloak" anyway. It's just a fallback.

What I had in mind was a simple test that would focus how disabling/enabling NAC behaves. It should check whether it defaults to a correct theme when NAC is disabled and enabled, respectively.

We didn't have such test before because we never had such functionality. :)

vmuzikar · 2020-11-13T17:20:31Z

themes/src/main/resources/theme/keycloak.v2/account/src/package.json

@@ -1,7 +1,7 @@
 {
-  "name": "keycloak-preview",
+  "name": "keycloak.v2",


This comment is unrelated to this line.

There seems to be something wrong with the dist. It's ~50MB larger. I think the resources filtering doesn't work anymore.

Good catch. I'll check it out.

This comment is unrelated to this line.

There seems to be something wrong with the dist. It's ~50MB larger. I think the resources filtering doesn't work anymore.

Should be fixed now.

stianst

I'm good with the current approach. At some point we may want theme versions to be a more built-in feature, rather than hardcoded as it is here, but that's out of scope now.

I haven't done a deep review though, so expecting on others for that.

hmlnarik · 2020-11-16T15:58:45Z

https://keycloak-jenkins.com/job/universal-test-pipeline-server/2319/
https://keycloak-jenkins.com/job/universal-test-pipeline-adapters/1908/

ssilvert · 2020-11-16T21:18:38Z

https://keycloak-jenkins.com/job/universal-test-pipeline-server/2319/
https://keycloak-jenkins.com/job/universal-test-pipeline-adapters/1908/

@hmlnarik There are many failures, but I'm not sure what is expected?

vmuzikar · 2020-11-18T10:40:10Z

...gration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AbstractKeycloakTest.java

@@ -96,6 +98,7 @@
 */
 @RunWith(KcArquillian.class)
 @RunAsClient
+@DisableFeature(value = Profile.Feature.ACCOUNT2, skipRestart = true)


I don't know what happened but my test fixes from ssilvert#10 were somehow merged wrong. This line definitely shouldn't be here... @ssilvert Did you do some rebase of my changes or something?

Could you please fix it and re-run both pipelines?

vmuzikar · 2020-11-18T13:39:00Z

Triggered:
https://keycloak-jenkins.com/job/universal-test-pipeline-server/2324/
https://keycloak-jenkins.com/job/universal-test-pipeline-adapters/1913/

vmuzikar · 2020-11-19T09:46:26Z

Triggered:
https://keycloak-jenkins.com/job/universal-test-pipeline-server/2324/
https://keycloak-jenkins.com/job/universal-test-pipeline-adapters/1913/

There are many new test failures in the base testsuite and I don't know why. I need to investigate.

vmuzikar · 2020-11-19T10:47:58Z

Just a quick update. It seems the new test failures are caused by #7603 which was not present here before the recent rebase.
In any case, this means we'll have to re-run the test pipelines yet again (once I have a fix for this), i.e. it won't be ready today.

vmuzikar · 2020-11-19T16:14:08Z

Found the culprit. My initial assumption was wrong, it was not caused by any commit introduced by the recent rebase (sorry @hmlnarik :)). The problem was actually in DefaultThemeManager which @ssilvert modified on Friday (which is why I didn't run into it sooner). It used a constant to store the information whether NAC is enabled or not. This caused intermittent failures based on the test order etc.

Created a fix for it here: ssilvert#11.

Pipelines are running here, I'll check the results in the morning:
https://keycloak-jenkins.com/job/universal-test-pipeline-server/2329/
https://keycloak-jenkins.com/job/universal-test-pipeline-adapters/1917/
Not sure if we'll need to re-run them again, after my PR was merged and squashed into this one. WDYT @hmlnarik?

hmlnarik · 2020-11-19T16:19:53Z

Found the culprit. My initial assumption was wrong, it was not caused by any commit introduced by the recent rebase (sorry @hmlnarik :)). The problem was actually in DefaultThemeManager which @ssilvert modified on Friday (which is why I didn't run into it sooner). It used a constant to store the information whether NAC is enabled or not. This caused intermittent failures based on the test order etc.

Created a fix for it here: ssilvert#11.

Pipelines are running here, I'll check the results in the morning:
https://keycloak-jenkins.com/job/universal-test-pipeline-server/2329/
https://keycloak-jenkins.com/job/universal-test-pipeline-adapters/1917/
Not sure if we'll need to re-run them again, after my PR was merged and squashed into this one. WDYT @hmlnarik?

Thanks @vmuzikar for your investigation! As long as the contents of the patch in this PR is the same as the one that you test against, I'm good to go without additional test pipelines.

ssilvert · 2020-11-19T18:33:03Z

Found the culprit. My initial assumption was wrong, it was not caused by any commit introduced by the recent rebase (sorry @hmlnarik :)). The problem was actually in DefaultThemeManager which @ssilvert modified on Friday (which is why I didn't run into it sooner). It used a constant to store the information whether NAC is enabled or not. This caused intermittent failures based on the test order etc.
Created a fix for it here: ssilvert#11.
Pipelines are running here, I'll check the results in the morning:
https://keycloak-jenkins.com/job/universal-test-pipeline-server/2329/
https://keycloak-jenkins.com/job/universal-test-pipeline-adapters/1917/
Not sure if we'll need to re-run them again, after my PR was merged and squashed into this one. WDYT @hmlnarik?

Thanks @vmuzikar for your investigation! As long as the contents of the patch in this PR is the same as the one that you test against, I'm good to go without additional test pipelines.

Yes. Mega kudos @vmuzikar!

When the time comes, does anyone object to doing "Squash and merge" instead of "Rebase and merge"?

I haven't gotten any pushback on the dev list about it yet. "Squash and merge" just seems less error-prone.

stianst · 2020-11-20T07:26:23Z

Squash and merge is fine

vmuzikar · 2020-11-20T08:10:40Z

I'm sorry, I made a mistake yesterday when triggering the pipelines. I was tired after the all-day bug hunt :) and I didn't realize I need to re-run the build pipeline as well (not just test pipelines with the server build from previous time) because the "test fix" was actually done in the server part, not test part.

I'm afraid we won't be able to merge this today as the pipelines take up to 12 hours to finish and it'll have to wait for the final approval till Monday.

vmuzikar · 2020-11-20T09:51:11Z

https://keycloak-jenkins.com/job/universal-test-pipeline-server/2331/
https://keycloak-jenkins.com/job/universal-test-pipeline-adapters/1919/

ssilvert · 2020-11-20T13:22:17Z

I'm sorry, I made a mistake yesterday when triggering the pipelines. I was tired after the all-day bug hunt :) and I didn't realize I need to re-run the build pipeline as well (not just test pipelines with the server build from previous time) because the "test fix" was actually done in the server part, not test part.

I'm afraid we won't be able to merge this today as the pipelines take up to 12 hours to finish and it'll have to wait for the final approval till Monday.

I'm actually really glad to hear that. I saw the failures and I worried we were back to square one. Thanks again for the effort.

vmuzikar

Final test results:

server
- cluster test failed due to some docker rate limit; re-ran here
- some DB tests failed due to the same error; I didn't re-run them as the same tests set was run with different DBs which should be enough in this case
adapters
- mod_auth_mellon tests failed but I re-ran them locally to avoid running the whole pipeline again (it's not possible to run selectively only mod_auth_mellon as all adapters tests share the same test script)

All in all, there seem to be no regressions! 🎉

I believe we're good to go.

ssilvert added Run UI Tests status/hold PR should not be merged. On hold for later. labels Jul 15, 2020

ssilvert self-assigned this Jul 15, 2020

ssilvert force-pushed the kc14547-make-nac-default-2 branch 7 times, most recently from 3f27201 to d8e95dc Compare July 20, 2020 16:45

ssilvert requested review from vmuzikar and stianst July 20, 2020 16:58

vmuzikar requested changes Jul 21, 2020

View reviewed changes

ssilvert force-pushed the kc14547-make-nac-default-2 branch from d8e95dc to 47fdc69 Compare July 22, 2020 21:47

vmuzikar requested changes Aug 6, 2020

View reviewed changes

abstractj requested changes Aug 17, 2020

View reviewed changes

ssilvert force-pushed the kc14547-make-nac-default-2 branch 2 times, most recently from 9562dae to c892721 Compare August 17, 2020 20:02

ssilvert force-pushed the kc14547-make-nac-default-2 branch 3 times, most recently from 8d143f1 to 734ffb8 Compare November 13, 2020 13:08

vmuzikar requested changes Nov 13, 2020

View reviewed changes

ssilvert force-pushed the kc14547-make-nac-default-2 branch from 734ffb8 to 80197ab Compare November 14, 2020 18:31

stianst previously approved these changes Nov 16, 2020

View reviewed changes

ssilvert dismissed stianst’s stale review via 3779ebd November 16, 2020 14:00

ssilvert force-pushed the kc14547-make-nac-default-2 branch 2 times, most recently from 5e4c514 to 20bc962 Compare November 16, 2020 15:06

vmuzikar reviewed Nov 18, 2020

View reviewed changes

ssilvert force-pushed the kc14547-make-nac-default-2 branch from 20bc962 to f1079d0 Compare November 18, 2020 12:45

vmuzikar previously approved these changes Nov 23, 2020

View reviewed changes

KEYCLOAK-14547: Make New Account Console the default.

f836a04

ssilvert dismissed vmuzikar’s stale review via f836a04 November 23, 2020 14:12

ssilvert force-pushed the kc14547-make-nac-default-2 branch from 05b768d to f836a04 Compare November 23, 2020 14:12

KEYCLOAK-14547: Make New Account Console the default.

df897ab

vmuzikar approved these changes Nov 23, 2020

View reviewed changes

stianst merged commit e56bd9d into keycloak:master Nov 23, 2020

	V1("v1");

	public static final AccountRestApiVersion DEFAULT = V1;

KEYCLOAK-14547: Make New Account Console the default. #7267

KEYCLOAK-14547: Make New Account Console the default. #7267

Conversation

ssilvert commented Jul 15, 2020 • edited by abstractj

vmuzikar commented Jul 20, 2020 • edited

ssilvert commented Jul 20, 2020

vmuzikar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmuzikar Jul 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmuzikar commented Aug 5, 2020 • edited

vmuzikar commented Aug 5, 2020

vmuzikar left a comment • edited

Choose a reason for hiding this comment

1. Broken CrossDC and Cluster tests

2. Node.js tests are broken

abstractj left a comment

Choose a reason for hiding this comment

ssilvert commented Aug 17, 2020

ssilvert commented Aug 17, 2020

1. Broken CrossDC and Cluster tests

2. Node.js tests are broken

vmuzikar commented Aug 20, 2020

vmuzikar commented Aug 20, 2020

ssilvert commented Aug 20, 2020

ssilvert commented Aug 20, 2020

vmuzikar commented Aug 20, 2020

vmuzikar left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stianst left a comment

Choose a reason for hiding this comment

hmlnarik commented Nov 16, 2020

ssilvert commented Nov 16, 2020

Choose a reason for hiding this comment

vmuzikar commented Nov 18, 2020

vmuzikar commented Nov 19, 2020

vmuzikar commented Nov 19, 2020

vmuzikar commented Nov 19, 2020

hmlnarik commented Nov 19, 2020

ssilvert commented Nov 19, 2020

stianst commented Nov 20, 2020

vmuzikar commented Nov 20, 2020 • edited

vmuzikar commented Nov 20, 2020

ssilvert commented Nov 20, 2020

vmuzikar left a comment • edited

Choose a reason for hiding this comment

ssilvert commented Jul 15, 2020 •

edited by abstractj

vmuzikar commented Jul 20, 2020 •

edited

vmuzikar Jul 28, 2020 •

edited

vmuzikar commented Aug 5, 2020 •

edited

vmuzikar left a comment •

edited

vmuzikar left a comment •

edited

vmuzikar commented Nov 20, 2020 •

edited

vmuzikar left a comment •

edited