Skip to content
This repository has been archived by the owner on Jun 5, 2024. It is now read-only.

Fetch settings values from settings service #475

Merged
merged 3 commits into from
May 22, 2020
Merged

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented May 20, 2020

This PR introduces a new settings class, which is responsible for fetching settings values from the settings service. It does so by using the generated JavaScript client (src/settingsClient.js). So please don't review src/settingsClient.js, as it's generated.

As said generated client relies on Axios, this PR also introduces Axios. The bearer token - if present - is injected in src/helperFunctions.js, so that axios requests can be made everywhere else in the code as well.

The function getSettingsValues from src/settings.js will be used in Phoenix. I will link a Phoenix PR here, soon. The code is supposed to fall back to defaults, if the settings service is unavailable. We'll have to discuss, if those defaults are supposed to live in the SDK or in Phoenix. But in my opinion that can be solved later on.

@kulmann kulmann self-assigned this May 20, 2020
@kulmann
Copy link
Member Author

kulmann commented May 20, 2020

At the moment the generated JS client for retrieving settings is using POST requests internally, for everything. We are working on changing this to something more RESTful.

@kulmann
Copy link
Member Author

kulmann commented May 20, 2020

Are you ok with moving the generated settings client into a vendor dir? Not so happy with it living in the src folder...

@LukasHirt
Copy link
Contributor

Are you ok with moving the generated settings client into a vendor dir? Not so happy with it living in the src folder...

Yeah, would be nice to make it clear this way that it's not something developers should edit.

src/settings.js Outdated Show resolved Hide resolved
src/settings.js Show resolved Hide resolved
src/settings.js Outdated Show resolved Hide resolved
src/settings.js Outdated Show resolved Hide resolved
src/settingsClient.js Outdated Show resolved Hide resolved
@kulmann kulmann changed the title PoC for fetching settings values Fetch settings values from settings service May 22, 2020
} catch (error) {
// fail on anything except settings service being unavailable
// TODO: if this changes to GET requests in the future, this will be 404 instead of 502
if (error.response.status !== 502) {

Choose a reason for hiding this comment

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

now thinking, there might also be a risk that if the service is just down (but configured) that Phoenix would revert to default values instead of showing an error. we can consider this case in a separate issue

Copy link

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

As discussed, just add the URL for the swagger-generated file and this is good to go 👍

@PVince81 PVince81 merged commit 080662f into master May 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the integrate-settings branch May 22, 2020 11:01
xoxys pushed a commit that referenced this pull request May 22, 2020
Merge: 369da93 262aa1b
Author: Vincent Petry <pvince81@owncloud.com>
Date:   Fri May 22 13:01:23 2020 +0200

    Merge pull request #475 from owncloud/integrate-settings

    Fetch settings values from settings service
xoxys pushed a commit that referenced this pull request May 22, 2020
Merge: 369da93 262aa1b
Author: Vincent Petry <pvince81@owncloud.com>
Date:   Fri May 22 13:01:23 2020 +0200

    Merge pull request #475 from owncloud/integrate-settings

    Fetch settings values from settings service
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants