Skip to content

Conversation

dfennessy
Copy link
Contributor

@dfennessy dfennessy commented Jun 23, 2022

Work done for Jira Issue THREESCALE-7919

Reviewers

@openshift-ci openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 23, 2022
@dfennessy dfennessy changed the title THREESCALE-7919-Added content for the DestinationRule custom resource. THREESCALE-7919: Added content for the DestinationRule custom resource. Jun 23, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2022
@dfennessy dfennessy force-pushed the 7919-ProvideDestinationRuleServiceEntry branch from 04d329a to c8ef0e3 Compare June 23, 2022 11:22
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2022
Copy link

@rahulanand16nov rahulanand16nov left a comment

Choose a reason for hiding this comment

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

LGTM

@dfennessy
Copy link
Contributor Author

Thanks @rahulanand16nov 😀

Copy link

@azgabur azgabur left a comment

Choose a reason for hiding this comment

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

LGTM

@JStickler
Copy link
Contributor

This PR needs a preview. The automated preview is currently disabled. The OpenShift Docs Manual has steps for how to create a manual preview here -> https://docs.google.com/document/d/1fLMpK4bqthtFlCwA36yeo2cI-pExFpX76VMnSi_In6Q/edit#heading=h.vrlifsbvvjd8

Copy link
Contributor

@JStickler JStickler left a comment

Choose a reason for hiding this comment

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

Couple of review comments. Also please create a preview link for this PR.

.Procedure

* Apply the following external `ServiceEntry` custom resources to your cluster:
* Apply the following external `ServiceEntry` and related `DestinationRule` custom resources to your cluster:
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a procedure, shouldn't this be formatted as numbered steps instead of a bulleted list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a single step. Does a single step need numbering?

Copy link
Contributor

Choose a reason for hiding this comment

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

Updating four different files is a single step? Unless you can do this with a single command, I'd say this is four different steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @JStickler

mode: SIMPLE
sni: multitenant.3scale.net
----
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 84 - The flow here might be better if you said something like, "Use the following command to apply external ServiceEntry and related DestinationRule custom resources to your cluster." list the commands, and the after the commands list the YAML files.

Line 100 "To do this, change the location of these services in the custom resources." these services is vague, we should be clear about which services should be moved to achieve the goal here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @JStickler

@rahulanand16nov
Can I get clarification on the following lines regarding "these services":

Alternatively, you can deploy an in-mesh 3scale service. To do this, change the location of these services in the CRs.

Choose a reason for hiding this comment

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

@dfennessy Here, we are using the URL for the 3scale SaaS version, but it doesn't have to be; users can deploy their 3scale and use links to that deployment.

@dfennessy
Copy link
Contributor Author

I'm abandoning this PR. Please close it.

I came in this morning and started working on it and faced enumerable conflicts that are wasting my time.

I've cloned the repo and I'm starting again.

@dfennessy
Copy link
Contributor Author

I'm abandoning this PR. Please close it.

I came in this morning and started working on it and faced enumerable conflicts that are wasting my time.

I've cloned the repo and I'm starting again.

@JStickler Please close this. I've created another PR to replace it with the changes except where I've questions for @rahulanand16nov

I'll update the new PR tomorrow: #47204

I am also having trouble manually creating the Preview using the steps in the Google doc. I will tackle this tomorrow as well.

@JStickler
Copy link
Contributor

@dfennessy You might want to ping Gabriel McGoldrick or Padraig O'Grady to ask for help with the preview builds, they're both OpenShift writers in your geo. Paul Needle also might be a good resource.

@dfennessy
Copy link
Contributor Author

@dfennessy You might want to ping Gabriel McGoldrick or Padraig O'Grady to ask for help with the preview builds, they're both OpenShift writers in your geo. Paul Needle also might be a good resource.

Thanks, @JStickler I will do that.

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

Labels

size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants