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.

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

[ELY-2544] [ELY-2254] Provide a LoginModule compatible security realm. #1931

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

Skyllarr
Copy link
Contributor

@Skyllarr Skyllarr commented Jul 17, 2023

issue: https://issues.redhat.com/browse/ELY-2544

This issue is resolved on maintenance branch by backporting the issue ELY-2254 to the maintenance branch.
Upstream branch does not need any changes.

This PR is only a backporting of ELY-2254

Upstream PR: #1623

@Skyllarr Skyllarr marked this pull request as ready for review July 26, 2023 12:23
@yersan
Copy link
Contributor

yersan commented Sep 20, 2023

It looks like PR links are wrong: PR title and commit message are related to ELY-2254, however, the description is linked to ELY-2544

@Skyllarr
Copy link
Contributor Author

Hello @yersan , this issue (ELY-2254) is meant to backport the changes of ELY-2544 to the 1.15.x maintanence branch. So the PR links are correct here

@yersan
Copy link
Contributor

yersan commented Sep 28, 2023

Hi @Skyllarr , sorry, I guess the title and Jira link in the description confused me.

this issue (ELY-2254) is meant to backport the changes of ELY-2544 to the 1.15.x maintanence branch. So the PR links are correct here.

I am not familiar with how Elytron manages its issues. On WildFly / WildFly Core we follow the rule where each PR commit, PR title, and PR Jira link match all together for each PR. Hence my confusion with this one, where the commit message and PR title use ELY-2254 but the Jira link is ELY-2544.

You mentioned this issue is ELY-2254, however, that issue is already resolved and since ELY-2544 Jira is still open, I understand that this PR is for ELY-2544. Ideally, ELY-2544 should be in the pull request sent with a link to this PR. Don't get me wrong, I am not asking to change anything now, just wanted to mention that it could confuse people outside of Elytron who are used to working on WildFly (like me :)). In any case, now I have a better picture of Jira links on EAP7-2068.

@Skyllarr Skyllarr changed the title [ELY-2254] Provide a LoginModule compatible security realm. [ELY-2544] [ELY-2254] Provide a LoginModule compatible security realm. Oct 5, 2023
@Skyllarr
Copy link
Contributor Author

Skyllarr commented Oct 5, 2023

Hi @yersan , I understand the confusion and I'm sorry about that. You're right, this issue was not linked in the ELY-2544 jira issue which was my mistake, it is now linked.

It is confusing because the ELY-2544 issue is part of a maintenance RFE where the only change needed in elytron was to backport the ELY-2254 to the maintenance branch (similar issue numbers do not help either :D ) And since this is a backport, the commit title remained ELY-2254. The upstream branch specifically does not need any elytron changes for the EAP7-2068 RFE.

I have now added both issue numbers to the PR title to make it more clear, and will also add the explanation to the Jira issue description. Thanks a lot for looking into this!

@fjuma
Copy link
Contributor

fjuma commented Nov 15, 2023

@Skyllarr I just realized that this PR just backports ELY-2254, there's nothing new being introduced for ELY-2544. I'll mark my previous comments as resolved.

Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

Approving as a backport, if there is anything specific to the backport we have missed let us know otherwise looks good.

@darranl darranl added the +1 DAL label Nov 22, 2023
@Skyllarr Skyllarr merged commit cc45576 into wildfly-security:1.15.x Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants