Skip to content

Conversation

dfennessy
Copy link
Contributor

@dfennessy dfennessy commented Oct 9, 2020

This pull requested is based on THREESCALE-6109

Reviewers

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 9, 2020
@neal-timpe neal-timpe self-requested a review October 9, 2020 13:52
Copy link

@unleashed unleashed left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a couple of suggestions.

@dfennessy dfennessy force-pushed the 6109-3scaleIstioServiceMesh2 branch from b74cd90 to 579ae03 Compare October 13, 2020 16:17
@dfennessy dfennessy force-pushed the 6109-3scaleIstioServiceMesh2 branch from 579ae03 to 51f70f2 Compare October 14, 2020 16:35
@neal-timpe
Copy link
Contributor

neal-timpe commented Oct 15, 2020

@dfennessy I'll add some reviewers. I noticed the build failure. The link xref:../../service_mesh/v2x/installing-ossm.adoc#ossm-control-plane-deploy-1x_installing-ossm-v1x[Deploying the Red Hat OpenShift Service Mesh control plane] should be xref:../../service_mesh/v2x/installing-ossm.adoc#ossm-control-plane-deploy_installing-ossm.

@brian-avery
Copy link
Contributor

LGTM. A couple of minor comments.

@dfennessy dfennessy force-pushed the 6109-3scaleIstioServiceMesh2 branch 3 times, most recently from b86eed9 to 3acdd49 Compare October 22, 2020 15:41
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 22, 2020
@fbolton fbolton force-pushed the 6109-3scaleIstioServiceMesh2 branch from 14a58c3 to 91a6c7b Compare October 23, 2020 14:49
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there are two versions of this file, so the "include" note needs to be updated that this one only applies to OSSM v1x.

Copy link

Choose a reason for hiding this comment

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

Ok, I just removed the second of these comments (on line 4) in my recent commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 4 - Since there are now two versions of this file, the "include" note needs to be updated that this one only applies to OSSM v2x.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a new module.
1 - Do you want to change the file name to ossm-threescale-backend-cache so that the file shows up in the list with your other files?
2 - Since this is a new file, it is not included in service_mesh/v1x/threescale_adapter/threescale-adapter.adoc

Copy link
Contributor

@JStickler JStickler Oct 23, 2020

Choose a reason for hiding this comment

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

This is a hard link to a version of the docs that does NOT include these parameters.

I think what you want here is an XREF to the file that will include the example with these new parameters.
The format is: xref:<relative_path_including_OSSM_version/<assembly_file_name>#underscore< context> [linktext]
so,
xref:../../service_mesh/v2x/threescale-adapter.adoc#ossm-cr-threescale_threescale-adapter[Example 3scale configuration options].

Copy link
Contributor

Choose a reason for hiding this comment

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

This XREF must be fixed before this PR can be merged. (my other commends are "nice to have")

Copy link

Choose a reason for hiding this comment

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

I changed the link to use the form of XREF that you recommended here. But I was not able to check the link locally, as I am not familiar with the build process for this document.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a new module.
1 - Do you want to change the file name to ossm-threescale-itstio-adapter-apicast so that the file shows up in the list with your other files?
2 - Since this is a new file, it is not included in service_mesh/v1x/threescale_adapter/threescale-adapter.adoc

Copy link

@unleashed unleashed left a comment

Choose a reason for hiding this comment

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

This needs changes in the text talking about OIDC - @pehala will provide a PR upstream we can incorporate here. /cc @dfennessy

Edit: here's the PR 3scale/3scale-istio-adapter#177

@fbolton
Copy link

fbolton commented Oct 29, 2020

@unleashed Ok, I just made the corresponding updates to the modules/ossm-threescale-authentication.adoc file based on Petr Hala's updates to the README file. Can you check if this looks Ok?

Copy link

@unleashed unleashed left a comment

Choose a reason for hiding this comment

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

Looks good @fbolton

@fbolton fbolton force-pushed the 6109-3scaleIstioServiceMesh2 branch from 2397d4f to 485ed96 Compare October 30, 2020 11:16
@fbolton
Copy link

fbolton commented Oct 30, 2020

@neal-timpe I just squashed the commits. I think we are ready to merge this PR now.

@JStickler JStickler merged commit 413afab into openshift:master Oct 30, 2020
@JStickler
Copy link
Contributor

JStickler commented Oct 30, 2020

/cherry-pick enterprise-4.6

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Oct 30, 2020

@JStickler: new pull request created: #26935

In response to this:

/cherry-pick enterprise-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@JStickler
Copy link
Contributor

JStickler commented Oct 30, 2020

/cherry-pick enterprise-4.7

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Oct 30, 2020

@JStickler: new pull request created: #26936

In response to this:

/cherry-pick enterprise-4.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants