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

add more config doc descriptions #3973

Merged
merged 14 commits into from
Jun 29, 2022
Merged

add more config doc descriptions #3973

merged 14 commits into from
Jun 29, 2022

Conversation

wkloucek
Copy link
Contributor

@wkloucek wkloucek commented Jun 15, 2022

Description

adds more config doc descriptions, removes unused configuration options

Related Issue

Motivation and Context

have all config options described

How Has This Been Tested?

  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@wkloucek wkloucek marked this pull request as ready for review June 21, 2022 13:16
@wkloucek wkloucek requested review from mmattel and removed request for kulmann and pascalwengerter June 21, 2022 13:17
Co-authored-by: Phil Davis <phil@jankaritech.com>
@owncloud owncloud deleted a comment from update-docs bot Jun 23, 2022
@owncloud owncloud deleted a comment from ownclouders Jun 23, 2022
@phil-davis
Copy link
Contributor

"This branch is 4 commits ahead, 39 commits behind owncloud:master."

Will that matter? Is it worth rebasing to make sure that it's all good?

@phil-davis phil-davis self-requested a review June 23, 2022 07:58
@ownclouders
Copy link
Contributor

💥 Acceptance test Core-API-Tests-ocis-storage-10 failed. Further test are cancelled...

@wkloucek wkloucek requested a review from mmattel June 28, 2022 11:06
Comment on lines 43 to 44
GatewayAddress string `yaml:"gateway_addr" env:"STORAGE_GATEWAY_GRPC_ADDR" desc:"GRPC address of the storage-system extension."`
StorageAddress string `yaml:"storage_addr" env:"STORAGE_GRPC_ADDR" desc:"GRPC address of the storage-system extension."`
Copy link
Contributor

Choose a reason for hiding this comment

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

These have the same description - are there 2 variables that do the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's a somehow weird decision to expose both settings. I think this grew organically and has historical reasons.

Is now tracked in #4058

services/sharing/pkg/config/config.go Outdated Show resolved Hide resolved
services/storage-system/pkg/config/config.go Outdated Show resolved Hide resolved
services/storage-users/pkg/config/config.go Outdated Show resolved Hide resolved
services/storage-users/pkg/config/config.go Outdated Show resolved Hide resolved
services/web/pkg/config/http.go Outdated Show resolved Hide resolved
@mmattel
Copy link
Contributor

mmattel commented Jun 28, 2022

@wkloucek see the image below, it seems that in the web service, one config var errors. It is between WEB_UI_CONFIG_SERVER and WEB_UI_CONFIG_VERSION. I do not know if this is fixed here, but if not pls take a look, thx.

Comment on lines +11 to 12

This service provides search functionality.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is obvious that a search service provides search functionality, but could we have a bit more meat on the bone 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please let us do abstracts in subsequent PRs. They should be more than one sentence.

Comment on lines 24 to 25
CommitShareToStorageGrant bool `yaml:"commit_share_to_storage_grant" env:"GATEWAY_COMMIT_SHARE_TO_STORAGE_GRANT" desc:"Commit shares to storage grants."`
CommitShareToStorageRef bool `yaml:"commit_share_to_storage_ref" env:"GATEWAY_COMMIT_SHARE_TO_STORAGE_REF" desc:"Commit shares to storage."`
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference in between the two? Or what is the impact is setting true/false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GATEWAY_COMMIT_SHARE_TO_STORAGE_REF was removed and the description updated in 4fe5703

Namespace string `yaml:"-"`
Root string `yaml:"root" env:"GRAPH_HTTP_ROOT"`
Root string `yaml:"root" env:"GRAPH_HTTP_ROOT" desc:"The root path of the HTTP service."`
Copy link
Contributor

Choose a reason for hiding this comment

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

The default is: /graph. In which folder can /graph be found?

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarified with @dragonchaser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to Subdirectory that serves as the root for this HTTP service. for all services that have that option.

if you have oCIS on https://ocis.owncloud.test, the graph service is available on https://ocis.owncloud.test/graph. If you want to have it on another subdirectory, eg. /foobar, you could change that config and also the proxy route to serve the graph on https://ocis.owncloud.test/foobar

Co-authored-by: Martin <github@diemattels.at>
Co-authored-by: Phil Davis <phil@jankaritech.com>
Copy link
Contributor

@mmattel mmattel left a comment

Choose a reason for hiding this comment

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

Approving, lets merge this and start a new PR to fix open/remaining/improvable stuff.

(But it would be great if you have the time to look at the one open env-question above before merging so it will not get "lost"...)

@wkloucek
Copy link
Contributor Author

@wkloucek see the image below, it seems that in the web service, one config var errors. It is between WEB_UI_CONFIG_SERVER and WEB_UI_CONFIG_VERSION. I do not know if this is fixed here, but if not pls take a look, thx.

Already fixed in this PR

@mmattel
Copy link
Contributor

mmattel commented Jun 29, 2022

@phil-davis 👍 😄

@sonarcloud
Copy link

sonarcloud bot commented Jun 29, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@wkloucek wkloucek merged commit 8a77ae8 into owncloud:master Jun 29, 2022
@wkloucek wkloucek deleted the config-doc-descriptions branch June 29, 2022 08:41
ownclouders pushed a commit that referenced this pull request Jun 29, 2022
Merge: ee0b466 4fe5703
Author: Willy Kloucek <34452982+wkloucek@users.noreply.github.com>
Date:   Wed Jun 29 10:41:06 2022 +0200

    Merge pull request #3973 from wkloucek/config-doc-descriptions

    add more config doc descriptions
@mmattel
Copy link
Contributor

mmattel commented Jun 29, 2022

👍

@micbar micbar mentioned this pull request Jul 19, 2022
36 tasks
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.

5 participants