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.

Sign up for GitHub

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

Jump to bottom

Add more details in guideline for Auth Istio&Apisix #485

Merged
merged 3 commits into from
Jan 22, 2025
Merged

Add more details in guideline for Auth Istio&Apisix #485

merged 3 commits into from
Jan 22, 2025

Conversation

XinyaoWa
Copy link
Contributor

@XinyaoWa XinyaoWa commented Oct 18, 2024 •
edited
Loading

Description

Add more details in guideline for Auth Istio&Apisix

Issues

n/a.

Type of change

List the type of change like below. Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would break existing design and interface)

Dependencies

no

Tests

Describe the tests that you ran to verify your changes.

@XinyaoWa XinyaoWa requested a review from Ruoyu-y as a code owner October 18, 2024 10:29
@XinyaoWa
Copy link
Contributor Author

@ckhened Please help to have a review~

Ruoyu-y
Ruoyu-y requested changes Nov 4, 2024
View reviewed changes
authN-authZ/auth-istio/README.md Show resolved Hide resolved
authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-apisix/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-apisix/keycloak_install.yaml Outdated Show resolved Hide resolved
@Ruoyu-y Ruoyu-y requested a review from ckhened November 4, 2024 01:44
@XinyaoWa XinyaoWa requested a review from Ruoyu-y November 6, 2024 03:34
Ruoyu-y
Ruoyu-y reviewed Nov 6, 2024
View reviewed changes
authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-istio/README.md Show resolved Hide resolved
authN-authZ/auth-apisix/keycloak_install.yaml Outdated Show resolved Hide resolved
Ruoyu-y
Ruoyu-y approved these changes Nov 19, 2024
View reviewed changes
Copy link
Collaborator

@Ruoyu-y Ruoyu-y left a comment

Choose a reason for hiding this comment

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

LGTM

@lianhao lianhao requested a review from mkbhanda November 28, 2024 02:33
@joshuayao joshuayao added this to the v1.2 milestone Jan 7, 2025
@joshuayao joshuayao linked an issue Jan 7, 2025 that may be closed by this pull request
[Feature] Istio m/TLS integration #548
Closed
@joshuayao joshuayao removed a link to an issue Jan 7, 2025
[Feature] Istio m/TLS integration #548
Closed
@joshuayao joshuayao requested review from ckhened and Ruoyu-y January 17, 2025 03:27
mkbhanda
mkbhanda reviewed Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@mkbhanda mkbhanda left a comment

Choose a reason for hiding this comment

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

Please consistently write apisix in text as APISIX, that is how it is on their site. @chickenrae comment -- may want to talk to @yongfengdu - what/how. Please number the steps for installing/configuring keycloak, it helps people. Please add a statement to the effect that envsubst is needed only on the machine from which one is launching the kubectl commands, it is not needed on any of the kubernetes cluster machines.

authN-authZ/auth-apisix/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-apisix/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-apisix/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-apisix/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-apisix/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-istio/README.md Show resolved Hide resolved
@poussa
Copy link
Collaborator

poussa commented Jan 17, 2025

Looks like we need more time to adapt all the changes, so this will not make v1.2 release.

@XinyaoWa XinyaoWa force-pushed the auth branch 4 times, most recently from 0e89d91 to 9ad2d39 Compare January 20, 2025 07:49
@XinyaoWa
Copy link
Contributor Author

Please consistently write apisix in text as APISIX, that is how it is on their site. @chickenrae comment -- may want to talk to @yongfengdu - what/how. Please number the steps for installing/configuring keycloak, it helps people. Please add a statement to the effect that envsubst is needed only on the machine from which one is launching the kubectl commands, it is not needed on any of the kubernetes cluster machines.

All fixed according to your suggestion~

@XinyaoWa
Copy link
Contributor Author

@mkbhanda Thanks for your careful review, I have updated according to your suggestion, please have a review again.

@XinyaoWa
Copy link
Contributor Author

Looks like we need more time to adapt all the changes, so this will not make v1.2 release.

Thanks for your aware for this PR, I have updated according to comments and think this PR can still target v1.2. Please have a review~

@XinyaoWa XinyaoWa requested review from ckhened and mkbhanda and removed request for ckhened January 20, 2025 08:00
@joshuayao
Copy link
Collaborator

Hi @mkbhanda @ckhened , could you please help review the changes? Thanks.

mkbhanda
mkbhanda reviewed Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@mkbhanda mkbhanda left a comment

Choose a reason for hiding this comment

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

A few minor edits more. Thank you for addressing all earlier comments.

authN-authZ/auth-apisix/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-apisix/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-istio/README.md Show resolved Hide resolved
@XinyaoWa
Copy link
Contributor Author

Hi @mkbhanda thanks for your new comments, I have all fixed, please have a review again~

XinyaoWa and others added 3 commits January 22, 2025 15:40
@XinyaoWa @lianhao
Add more details in guideline for Auth Istio&Apisix
179b782 
Signed-off-by: Xinyao Wang <xinyao.wang@intel.com>
@XinyaoWa @lianhao
refine readme according to comments
cbe845d 
Signed-off-by: Xinyao Wang <xinyao.wang@intel.com>
@pre-commit-ci @lianhao
[pre-commit.ci] auto fixes from pre-commit.com hooks
2fd2575 
for more information, see https://pre-commit.ci
mkbhanda
mkbhanda approved these changes Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@mkbhanda mkbhanda left a comment

Choose a reason for hiding this comment

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

LGTM

@mkbhanda mkbhanda merged commit 6b04b84 into opea-project:main Jan 22, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ruoyu-y Ruoyu-y Ruoyu-y approved these changes

@mkbhanda mkbhanda mkbhanda approved these changes

@ckhened ckhened Awaiting requested review from ckhened

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
6 participants
@XinyaoWa @poussa @joshuayao @mkbhanda @ckhened @Ruoyu-y