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.

Sign up for GitHub

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

Jump to bottom

Update theme path in ocis-wopi, ocis-hello examples, drone config and default flagset docs #2437

Merged
merged 3 commits into from
Oct 12, 2021
Merged

Update theme path in ocis-wopi, ocis-hello examples, drone config and default flagset docs #2437

merged 3 commits into from
Oct 12, 2021

Conversation

pascalwengerter
Copy link
Contributor

Description

Current (outdated) config relies on fallback

@update-docs
Copy link

update-docs bot commented Aug 25, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@wkloucek
Copy link
Contributor

But that means our default in oCIS is also invalid!? -> https://owncloud.dev/extensions/web/configuration/

-web-config-theme | $WEB_UI_CONFIG_THEME
Theme. Default: "owncloud".

@pascalwengerter
Copy link
Contributor Author

But that means our default in oCIS is also invalid!? -> https://owncloud.dev/extensions/web/configuration/

-web-config-theme | $WEB_UI_CONFIG_THEME
Theme. Default: "owncloud".

Most definitely yes. Also not sure if it takes precedence over whatever is written in the config file (which from my POV would be expected behavior)

@pascalwengerter pascalwengerter force-pushed the update-deployment-example-configs branch 2 times, most recently from 522b68c to c456fcd Compare September 28, 2021 10:27
@pascalwengerter
Copy link
Contributor Author

@wkloucek rebased, found another broken example of the theme path in the drone config (fails silently with a sane fallback but 1 less console error), fixed the flagset default. Could you re-review and merge if happy?

@pascalwengerter pascalwengerter changed the title [docs-only] Update theme configuration in ocis-wopi, ocis-hello examples [docs-only] Update theme path in ocis-wopi, ocis-hello examples, drone config and default flagset docs Sep 28, 2021
wkloucek
wkloucek requested changes Sep 28, 2021
View reviewed changes
web/pkg/flagset/flagset.go Outdated
@@ -170,7 +170,7 @@ func ServerWithConfig(cfg *config.Config) []cli.Flag {
},
&cli.StringFlag{
Name: "web-config-theme",
Value: flags.OverrideDefaultString(cfg.Web.Config.Theme, "owncloud"),
Value: flags.OverrideDefaultString(cfg.Web.Config.Theme, "https://localhost:9200/themes/owncloud/theme.json"),
Copy link
Contributor

Choose a reason for hiding this comment

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

we at least need to use OCIS_URL in that case as a fallback URL. Are relative paths also supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

which actually gets quite ugly because OCIS_URL ist just expected to have the protocol+host part (eg. https://localhost:9200)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's your suggestion? The default is "check in the config file for web" which goes looking at whatever path is provided and if that one fails it loads the default theme we ship inside web...

Copy link
Contributor

Choose a reason for hiding this comment

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

@refs do you have an idea how to solve this? Currently my only idea would be to default to an empty string and care for the default later in the code... But that breaks config files and cli options.

Is this a case we considered already in #2024 ?

Copy link
Member

Choose a reason for hiding this comment

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

Just so I'm on the same page, the semantics of $WEB_UI_CONFIG_THEME, as I gather from the above discussion, is an endpoint where web downloads the theme? If that is the case we have to rename it to $WEB_UI_CONFIG_THEME_URL, since web expects a URL (in the strict definition of URL, which means no relative paths, since it is not a resource...)

Or else it looks to me that we have 2 config values to build the path that @wkloucek described above:

"https://localhost:9200/themes/owncloud/theme.json" is the combination of:

{$WEB_UI_CONFIG_SERVER || $OCIS_URL}/themes/{$WEB_UI_CONFIG_THEME}/theme.json

And both of them have the default set so the end result is a URL that @wkloucek described above. But these changes probably go beyond the scope of this ticket since it is docs-only 😸

Copy link
Contributor

Choose a reason for hiding this comment

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

I just pushed a possible solution as a commit to this PR. @pascalwengerter please have a look if this is ok to you

@wkloucek wkloucek changed the title [docs-only] Update theme path in ocis-wopi, ocis-hello examples, drone config and default flagset docs Update theme path in ocis-wopi, ocis-hello examples, drone config and default flagset docs Oct 12, 2021
pascalwengerter
pascalwengerter commented Oct 12, 2021
View reviewed changes
Copy link
Contributor Author

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Can't properly approve since it's my own PR but apart from a small nitpick this looks good to me!

web/pkg/flagset/flagset.go Outdated Show resolved Hide resolved
@wkloucek wkloucek self-requested a review October 12, 2021 13:53
wkloucek
wkloucek approved these changes Oct 12, 2021
View reviewed changes
Copy link
Contributor

@wkloucek wkloucek left a comment

Choose a reason for hiding this comment

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

LGTM

@pascalwengerter
Copy link
Contributor Author

@wkloucek CI is very unhappy, could you check locally if the code is correctly formatted and unit tests pass?

@wkloucek
Copy link
Contributor

@wkloucek CI is very unhappy, could you check locally if the code is correctly formatted and unit tests pass?

just a old starlark... Will rebase

@wkloucek wkloucek force-pushed the update-deployment-example-configs branch from 7a5786d to 987261f Compare October 12, 2021 14:27
@sonarcloud
Copy link

sonarcloud bot commented Oct 12, 2021

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 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@wkloucek wkloucek merged commit 60eb304 into master Oct 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the update-deployment-example-configs branch October 12, 2021 15:23
ownclouders pushed a commit that referenced this pull request Oct 12, 2021
@wkloucek
commit 60eb304
feaf3d9 
Merge: 90246d7 987261f
Author: Willy Kloucek <34452982+wkloucek@users.noreply.github.com>
Date:   Tue Oct 12 17:23:00 2021 +0200

    Merge pull request #2437 from owncloud/update-deployment-example-configs

    Update theme path in ocis-wopi, ocis-hello examples, drone config and default flagset docs
wkloucek added a commit that referenced this pull request Oct 14, 2021
@wkloucek
fix invalid json:"omit" added in #2437
9f28316
This was referenced Oct 14, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refs refs refs left review comments

@wkloucek wkloucek wkloucek approved these changes

@kulmann kulmann Awaiting requested review from kulmann

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pascalwengerter @wkloucek @refs