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

Fix numerous doc issues #651

Merged
merged 2 commits into from
Jan 28, 2021
Merged

Fix numerous doc issues #651

merged 2 commits into from
Jan 28, 2021

Conversation

Signed-off-by: Anatolii Bazko <[email protected]>
// +optional
DevfileRegistryURL string `json:"devfileRegistryURL"`
// Public URL to the Plugin registry
// Public URL to the Plugin registry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "Plugin" should start with lowercase too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right

// If both are defined, `serverExposureStrategy` takes precedence.
// Strategy for ingress creation. Options are: `multi-host` (host is explicitly provided in ingress),
// `single-host` (host is provided, path-based rules) and `default-host` (no host is provided, path-based rules).
// Defaults to `multi-host` Deprecated in favor of `serverExposureStrategy` in the `server` section,
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr Jan 28, 2021

Choose a reason for hiding this comment

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

Defaults to multi-host

Should this sentence end with point '.' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I don't know. I applied suggested changes from https://github.com/tolusha/che-docs/pull/4/commits

// correspondly to auto-detection result.
// This property allows users to directly login with their Openshift user through the Openshift login,
// and have their workspaces created under personal OpenShift namespaces.
// Enabled by default on OpenShift. This will allow users to directly login with their OpenShift user through the OpenShift login,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have mecanism to auto-enable oauth. By default property is an empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

// Password of the proxy server
// Only use when proxy configuration is required
// (see also the `proxyURL`, `proxyUser` and `proxySecret` fields).
// Password of the proxy server Only use when proxy configuration is required (see also the `proxyURL`, `proxyUser` and `proxySecret` fields).
Copy link
Contributor

Choose a reason for hiding this comment

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

Password of the proxy server

Should be '.' in the end of the sentences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right.

// +optional
ProxyURL string `json:"proxyURL,omitempty"`
// Port of the proxy server. Only use when configuring a proxy is required.
// (see also the `proxyURL` and `nonProxyHosts` fields).
// Port of the proxy server. Only use when configuring a proxy is required. (see also the `proxyURL` and `nonProxyHosts` fields).
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr Jan 28, 2021

Choose a reason for hiding this comment

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

(see also the proxyURL and nonProxyHosts fields).

Looks strange. Maybe:

See also the `proxyURL` and `nonProxyHosts` fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right

// Defines if a user is able to specify Kubernetes namespace (or OpenShift project) different from the default.
// It's NOT RECOMMENDED to configured true without OAuth configured. This property is also used by the OpenShift infra.
// Defines that a user is allowed to specify Kubernetes namespace, or OpenShift project, different from the default.
// It's NOT RECOMMENDED to be configured true without OpenShift OAuth configured. The OpenShift infrastructure also uses this property.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's NOT RECOMMENDED to be configured true without OpenShift OAuth configured.

Maybe we should change this sentence, because it uses 'configured' two times. Maybe:

It's NOT RECOMMENDED to set true value without OpenShift OAuth configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense

@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #651 (f0431bf) into master (fbe9a3e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #651   +/-   ##
=======================================
  Coverage   27.65%   27.65%           
=======================================
  Files          43       43           
  Lines        4672     4672           
=======================================
  Hits         1292     1292           
  Misses       3209     3209           
  Partials      171      171           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbe9a3e...f0431bf. Read the comment docs.

Signed-off-by: Anatolii Bazko <[email protected]>
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AndrienkoAleksandr, tolusha
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@tolusha tolusha merged commit 19e1332 into master Jan 28, 2021
@tolusha tolusha deleted the updatedoc branch January 28, 2021 12:41
@che-bot che-bot added this to the 7.26 milestone Jan 28, 2021
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.

4 participants