8000 Fixes using wrong db when using import or export command by DGuhr · Pull Request #12081 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Fixes using wrong db when using import or export command - #12081

Closed
DGuhr wants to merge 1 commit into
keycloak:mainfrom
DGuhr:iss11902_fix_importexport
Closed

Fixes using wrong db when using import or export command#12081
DGuhr wants to merge 1 commit into
keycloak:mainfrom
DGuhr:iss11902_fix_importexport

Conversation

@DGuhr
@DGuhr DGuhr commented May 18, 2022
Copy link
Copy Markdown
Contributor

Closes #11902
Closes #11767

@DGuhr
DGuhr force-pushed the iss11902_fix_importexport branch from 180b0ca to 6b04ffe Compare May 18, 2022 16:14
andreaTP
andreaTP previously approved these changes May 18, 2022
View reviewed changes
@andreaTP andreaTP left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@DGuhr
DGuhr force-pushed the iss11902_fix_importexport branch from 6b04ffe to b1a45ba Compare May 18, 2022 16:14
@DGuhr
DGuhr force-pushed the iss11902_fix_importexport branch from b1a45ba to 896430d Compare May 18, 2022 16:19
@stianst
stianst requested a review from pedroigor May 19, 2022 09:19
pedroigor
pedroigor reviewed May 23, 2022
View reviewed changes
@pedroigor pedroigor left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@DGuhr I think it is a fair assumption that import/export should always run in runtime mode. You probably have a pre-configured server if you are running import/export.

I'll run your PR and do some local tests too.

The only thing I'm not sure is the test. Why we can't have the test similar to how we are running the database tests?

@pedroigor
pedroigor commented May 23, 2022
Copy link
Copy Markdown
Contributor

@DGuhr After looking at this one in more detail, I noticed that the issue is somewhere else. Please, see #12156.

I also tried to simplify the tests a bit so that we don't need to introduce a new dependency but just leverage the existing code to run database tests. The export/import tests should also run for all the databases we are testing too (instead of running only postgres).

Please, let me know if my changes makes sense for you and if you want to incorporate them into this PR. Otherwise, I can also send a new PR.

@pedroigor pedroigor mentioned this pull request May 24, 2022
Merged
@abstractj abstractj self-assigned this May 25, 2022
@abstractj
abstractj commented May 25, 2022
Copy link
Copy Markdown
Contributor

@pedroigor please correct me if I'm wrong, but it seems that the changes proposed here were superseded by #12157. Is that correct? If yes, I believe it's safe to close this PR.

@pedroigor
pedroigor commented May 25, 2022
Copy link
Copy Markdown
Contributor

@abstractj Yeah, you are right. I forgot about it. Closing.

@pedroigor
pedroigor commented May 25, 2022
Copy link
Copy Markdown
Contributor

Superseded by #12157.

@abstractj
abstractj commented May 25, 2022
Copy link
Copy Markdown
Contributor

@DGuhr closing it based on @pedroigor's comments.

@abstractj abstractj closed this May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pedroigor pedroigor pedroigor left review comments
+1 more reviewer
@andreaTP andreaTP andreaTP approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@abstractj abstractj

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Keycloak.X 18.0.0 - export/import in container does not work as expected Inconsistent configuration settings for start and import commands

4 participants

@DGuhr @pedroigor @abstractj @andreaTP
0