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

Hardcoded web options #6188

Merged
merged 4 commits into from
May 2, 2023
Merged

Conversation

kobergj
Copy link
Collaborator

@kobergj kobergj commented Apr 28, 2023

Fixes #4921

@kobergj kobergj requested a review from kulmann as a code owner April 28, 2023 10:35
@ownclouders
Copy link
Contributor

ownclouders commented Apr 28, 2023

💥 Acceptance test Web-Tests-ocis-smoke-ocis-storage-1 failed. Further test are cancelled...

@micbar micbar requested review from mmattel and wkloucek April 28, 2023 11:42
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.

LGTM from docs pov

kobergj and others added 2 commits April 28, 2023 15:08
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Co-authored-by: Martin <github@diemattels.at>
@micbar
Copy link
Contributor

micbar commented Apr 28, 2023

seems that we have some fallout here from the options.

@JammingBen Any ideas how that could break the selenium tests?

@JammingBen
Copy link
Contributor

https://host.docker.internal:9200/config.json returns a lot more options now.

Before:
Pasted Graphic 2

After:
Pasted Graphic 3

Note that options holds a lot more items. E.g. it now has sharingRecipientsPerPage: 0, which overwrites Web setting it to 200 per default. As a result, no sharing recipients will be shown -> failing pipeline.

Is it desired for oCIS to return all those values now? If so, the default values have to be implemented in oCIS I suppose.

@micbar
Copy link
Contributor

micbar commented Apr 28, 2023

@JammingBen Thanks for digging into it. We need to omit empty values to use the web defaults.

}

// XHR are the XHR options
type XHR struct {
Timeout int `json:"timeout" yaml:"timeout" desc:"Specifies the timeout for XHR uploads in milliseconds."`
Timeout int `json:"timeout,omitempty" yaml:"timeout" env:"WEB_OPTION_UPLOAD_XHR_TIMEOUT" desc:"Specifies the timeout for XHR uploads in milliseconds."`
Copy link
Contributor

Choose a reason for hiding this comment

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

this one was missing

@micbar
Copy link
Contributor

micbar commented Apr 28, 2023

@JammingBen @kobergj @mmattel
I added some defaults.

Ocis default is now

{
  "previewFileMimeTypes": [
    "image/gif",
    "image/png",
    "image/jpeg",
    "text/plain",
    "image/tiff",
    "image/bmp",
    "image/x-ms-bmp"
  ],
  "sharingRecipientsPerPage": 200,
  "sidebar": {
    "shares": {
      "showAllOnLoad": false
    }
  },
  "routing": {
    "idBased": true
  },
  "contextHelpersReadMore": true
}

@kulmann FYI

@micbar
Copy link
Contributor

micbar commented Apr 28, 2023

Some empty defaults are omitted on the json object. They are relying on the web default.

Please speak up if you think that we should make all of the defaults explicit.

@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2023

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@mmattel
Copy link
Contributor

mmattel commented Apr 29, 2023

Consider before merging:

Following point for ease of readbility/identification:
Current:
yaml:"href" env:"WEB_OPTION_FEEDBACKLINK_HREF"
yaml:"OpenAppsInTab" env:"WEB_OPTION_OPEN_APPS_IN_TAB"

For naming the envvars to identify its purpose we have set that they start with: WEB_OPTION_ which is good as we can identify them very easy. But looking for the yaml version, we "only" have href.

If one is looking for all options that configure the webui, not the web service itself, he can clearly identify them in the envvars but not in the yamls.

Proposal:
Start all yamls that are related to configure webui options start with option. This would ease identify them compared to that what we currently have in this PR like:
yaml:"optiontHref" env:"WEB_OPTION_FEEDBACKLINK_HREF"
yaml:"optionOpenAppsInTab" env:"WEB_OPTION_OPEN_APPS_IN_TAB"
...

This also would help documenting that there are options to configure the webui behaviour as I could then reference to a common name component in both the envvars table and the yaml table.

@micbar
Copy link
Contributor

micbar commented Apr 29, 2023

@mmattel The yaml is nested:

web:
  options:
     feedbacklink:
        href: https://www.example.com

@kulmann
Copy link
Member

kulmann commented May 2, 2023

Some empty defaults are omitted on the json object. They are relying on the web default.

Please speak up if you think that we should make all of the defaults explicit.

I would prefer to define the defaults implicitly in web. I need to define them there anyway because:

  • oc10 needs defaults as well
  • any newly introduced config option would need a roundtrip to the ocis repo first
  • I don't want to manage defaults in two repos (ocis and web)

wkloucek added a commit to owncloud/ocis-charts that referenced this pull request May 2, 2023
@mmattel
Copy link
Contributor

mmattel commented May 2, 2023

I would prefer to define the defaults implicitly in web

To make admins aware about the implicit setting made by the webui, we could add what is implicit set in the envvar description. Else, how does an admin know what is configured?

@kulmann
Copy link
Member

kulmann commented May 2, 2023

I would prefer to define the defaults implicitly in web

To make admins aware about the implicit setting made by the webui, we could add what is implicit set in the envvar description. Else, how does an admin know what is configured?

Mentioning the defaults from web in the env var descriptions is a good idea, yes.

@micbar
Copy link
Contributor

micbar commented May 2, 2023

Mentioning the defaults from web in the env var descriptions is a good idea, yes.

What is missing?

I added some.

@micbar
Copy link
Contributor

micbar commented May 2, 2023

The feedback link defaults are already mentioned. The rest is IMO part of the ocis defaults and thereby documented.

Probably missing:

  • what about the xhr timeout option @kulmann

@butonic butonic merged commit 44de288 into owncloud:master May 2, 2023
ownclouders pushed a commit that referenced this pull request May 2, 2023
* use custom struct for web options

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* update envvar descriptions

Co-authored-by: Martin <github@diemattels.at>

* Use correct defaults

* fix code style

---------

Signed-off-by: jkoberg <jkoberg@owncloud.com>
Co-authored-by: Michael Barz <michael.barz@zeitgestalten.eu>
Co-authored-by: Martin <github@diemattels.at>
Co-authored-by: Michael Barz <mbarz@owncloud.com>
@kulmann
Copy link
Member

kulmann commented May 2, 2023

  • what about the xhr timeout option @kulmann

Not relevant for oCIS as we use TUS there.

@micbar micbar mentioned this pull request May 3, 2023
89 tasks
@ScharfViktor ScharfViktor mentioned this pull request May 4, 2023
86 tasks
wkloucek added a commit to owncloud/ocis-charts that referenced this pull request May 8, 2023
wkloucek added a commit to owncloud/ocis-charts that referenced this pull request May 9, 2023
fschade pushed a commit that referenced this pull request Jul 10, 2023
* use custom struct for web options

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* update envvar descriptions

Co-authored-by: Martin <github@diemattels.at>

* Use correct defaults

* fix code style

---------

Signed-off-by: jkoberg <jkoberg@owncloud.com>
Co-authored-by: Michael Barz <michael.barz@zeitgestalten.eu>
Co-authored-by: Martin <github@diemattels.at>
Co-authored-by: Michael Barz <mbarz@owncloud.com>
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.

Update Web service configuration
7 participants