Skip to content

KEYCLOAK-15595: update keycloak js for KEYCLOAK-15595#8011

Closed
sanket-bhalerao wants to merge 1 commit intokeycloak:masterfrom
sanket-bhalerao:master
Closed

KEYCLOAK-15595: update keycloak js for KEYCLOAK-15595#8011
sanket-bhalerao wants to merge 1 commit intokeycloak:masterfrom
sanket-bhalerao:master

Conversation

@sanket-bhalerao
Copy link
Copy Markdown
Contributor

while working on cordova+angular+ios the keycloak logout is not working. as the user clicks logout the user can again see the app instead of the inappbrowser page for login.
with clearcache=yes in the inappbrowser open the issue appears no more.

mhajas
mhajas previously approved these changes May 10, 2021
Copy link
Copy Markdown
Contributor

@mhajas mhajas left a comment

Choose a reason for hiding this comment

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

I tested this on Android and it seems it didn't break anything. Let's wait for ios review, but from my point of view, this looks good.

@sanket-bhalerao
Copy link
Copy Markdown
Contributor Author

@mhajas any luck?

@abstractj abstractj self-assigned this Jun 21, 2021
@abstractj
Copy link
Copy Markdown
Contributor

@mhajas who is supposed to review the iOS part?

@mhajas
Copy link
Copy Markdown
Contributor

mhajas commented Jun 22, 2021

@abstractj. I asked @vmuzikar yesterday and he told me he will try to have a look this week.

vmuzikar
vmuzikar previously approved these changes Jun 23, 2021
Copy link
Copy Markdown
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

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

@sanket-bhalerao Thank you for your contribution and sorry for the late reply.

I can confirm this PR correctly fixes the issue. Tested it both on iOS simulator and a real iOS device. BTW, the issue was now reproducible even on the simulator (not just the real device like before).

@mhajas
Copy link
Copy Markdown
Contributor

mhajas commented Jun 24, 2021

Thank you very much @vmuzikar! @abstractj Could you please run github actions workflow and then possibly merge?

@sanket-bhalerao
Copy link
Copy Markdown
Contributor Author

@abstractj @mhajas @vmuzikar any luck?

1 similar comment
@sanket-bhalerao
Copy link
Copy Markdown
Contributor Author

@abstractj @mhajas @vmuzikar any luck?

@hmlnarik
Copy link
Copy Markdown
Contributor

GH has not allowed running the checks, thus closing and reopening.

@hmlnarik hmlnarik closed this Jul 21, 2021
@hmlnarik hmlnarik reopened this Jul 21, 2021
@abstractj
Copy link
Copy Markdown
Contributor

@mhajas @hmlnarik tests were retriggered by it seems we have some failing tests.

@mhajas
Copy link
Copy Markdown
Contributor

mhajas commented Jul 22, 2021

@abstractj I don't think this PR is causing any failures in Github actions as the code that is changed is not tested. Should we rebase to make the tests pass?

@abstractj
Copy link
Copy Markdown
Contributor

@mhajas we have 2 options:

  1. Ask @sanket-bhalerao to kindly rebase his PR
  2. Take over this PR and open a new one

It is not possible for me to only rebase.

@sanket-bhalerao
Copy link
Copy Markdown
Contributor Author

@abstractj @mhajas i have used fetch and merge option in the repository today. it included around 219 commits to my fork.
is this what you mean by rebase ?
image

@mhajas
Copy link
Copy Markdown
Contributor

mhajas commented Jul 30, 2021

@sanket-bhalerao Thank you for updating the branch. Unfortunately, the fetch and merge functionality on Github doesn't work as we would like. We prefer having PRs with only one commit so this merge commit shouldn't be there. Could you please squash your commits together so that there is only one?

@sanket-bhalerao
Copy link
Copy Markdown
Contributor Author

@mhajas i am not able to squash the commits, please let me know if i should create a new pull request ?

@mhajas
Copy link
Copy Markdown
Contributor

mhajas commented Jul 30, 2021

You should be able to do it locally, just run

git reset --soft HEAD~2 &&
git commit

then you need to do force push git push -f

@sanket-bhalerao
Copy link
Copy Markdown
Contributor Author

@mhajas let me try..

@sanket-bhalerao sanket-bhalerao dismissed stale reviews from vmuzikar and mhajas via 89b05a9 July 30, 2021 13:47
@sanket-bhalerao
Copy link
Copy Markdown
Contributor Author

@mhajas i think something is wrong with it now..
i followed the same 2 commands mentioned.. now its showing 1475 file modified..

@mhajas
Copy link
Copy Markdown
Contributor

mhajas commented Jul 30, 2021

Ahhh, I am really sorry. That is my fault, it wasn't the best idea to do it. I can open a PR for you or if you want to send it yourself you can close this one and send a new PR based on the latest master. It is also possible to force push it to this branch but should be easier to send a new PR.

@sanket-bhalerao
Copy link
Copy Markdown
Contributor Author

let me open a new PR..

@sanket-bhalerao
Copy link
Copy Markdown
Contributor Author

@mhajas opened a new pull requst #8335.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants