Show different error message if server config is invalid
If a code-owners operation cannot be performed because the project
contains an invalid OWNERS file the backend returns a '409 Conflict'
response informing about the invalid OWNERS file. An invalid OWNERS file
is a configuration issue that needs to be fixed by the project owner or
host admin, it's not an error in the code-owners plugin (an error would
be signaled as 500 response). At the moment the frontend reports invalid
OWNERS file as a red error banner. This gives the wrong impression that
there was an error in the code-owners plugin. The way how invalid OWNERS
files are reported/shown to the user is changed so that it's more clear
that this is a configuration issue and not an error in the plugin.
Change-Id: I0e2524cbd1c76f954b77aff19a0da06b78574aec
diff --git a/ui/code-owners-api.js b/ui/code-owners-api.js
index 408428b..a1a1aed 100644
--- a/ui/code-owners-api.js
+++ b/ui/code-owners-api.js
@@ -15,12 +15,10 @@
* limitations under the License.
*/
-
// TODO: Try to remove it. The ResponseError and getErrorMessage duplicates
// code from the gr-plugin-rest-api.ts. This code is required because
// we want custom error processing in some functions. For details see
// the original gr-plugin-rest-api.ts file/
-
class ResponseError extends Error {
constructor(response) {
super();
@@ -28,6 +26,12 @@
}
}
+export class ServerConfigurationError extends Error {
+ constructor(msg) {
+ super(msg);
+ }
+}
+
async function getErrorMessage(response) {
const text = await response.text();
return text ?
@@ -46,13 +50,39 @@
}
/**
+ * Send a get request and provides custom response-code handling
+ */
+ async _get(url) {
+ const errFn = (response, error) => {
+ if (error) throw error;
+ if (response) throw new ResponseError(response);
+ throw new Error('Generic REST API error');
+ };
+ try {
+ return await this.restApi.send(
+ 'GET',
+ url,
+ undefined,
+ errFn
+ );
+ } catch (err) {
+ if (err instanceof ResponseError && err.response.status === 409) {
+ return getErrorMessage(err.response).then(msg => {
+ throw new ServerConfigurationError(msg);
+ });
+ }
+ throw err;
+ }
+ }
+
+ /**
* Returns a promise fetching the owner statuses for all files within the change.
*
* @doc https://gerrit.googlesource.com/plugins/code-owners/+/refs/heads/master/resources/Documentation/rest-api.md#change-endpoints
* @param {string} changeId
*/
listOwnerStatus(changeId) {
- return this.restApi.get(`/changes/${changeId}/code_owners.status`);
+ return this._get(`/changes/${changeId}/code_owners.status`);
}
/**
@@ -63,7 +93,7 @@
* @param {string} path
*/
listOwnersForPath(changeId, path, limit) {
- return this.restApi.get(
+ return this._get(
`/changes/${changeId}/revisions/current/code_owners` +
`/${encodeURIComponent(path)}?limit=${limit}&o=DETAILS`
);
@@ -78,7 +108,7 @@
* @param {string} path
*/
getConfigForPath(project, branch, path) {
- return this.restApi.get(
+ return this._get(
`/projects/${encodeURIComponent(project)}/` +
`branches/${encodeURIComponent(branch)}/` +
`code_owners.config/${encodeURIComponent(path)}`
@@ -93,19 +123,11 @@
* @param {string} branch
*/
async getBranchConfig(project, branch) {
- const errFn = (response, error) => {
- if (error) throw error;
- if (response) throw new ResponseError(response);
- throw new Error('Generic REST API error');
- }
try {
- const config = await this.restApi.send(
- 'GET',
+ const config = await this._get(
`/projects/${encodeURIComponent(project)}/` +
`branches/${encodeURIComponent(branch)}/` +
- `code_owners.branch_config`,
- undefined,
- errFn
+ `code_owners.branch_config`
);
if (config.override_approval && !(config.override_approval
instanceof Array)) {
@@ -116,7 +138,7 @@
return {...config, override_approval: [config.override_approval]};
}
return config;
- } catch(err) {
+ } catch (err) {
if (err instanceof ResponseError) {
if (err.response.status === 404) {
// The 404 error means that the branch doesn't exist and
diff --git a/ui/code-owners-banner.js b/ui/code-owners-banner.js
index 4e4b07e..1aa9e58 100644
--- a/ui/code-owners-banner.js
+++ b/ui/code-owners-banner.js
@@ -15,7 +15,7 @@
* limitations under the License.
*/
import {CodeOwnersModelMixin} from './code-owners-model-mixin.js';
-import {PluginState} from './code-owners-model.js';
+import {PluginState, isPluginErrorState} from './code-owners-model.js';
// There are 2 elements in this file:
// CodeOwnersBanner - visual elements. This element is shown at the top
@@ -79,7 +79,7 @@
margin-left: var(--spacing-l);
}
</style>
- <span class="text">Error: Code-owners plugin has failed</span>
+ <span class="text">[[_getErrorText(pluginStatus)]]</span>
<gr-button link on-click="_showFailDetails">
Details
</gr-button>
@@ -112,7 +112,14 @@
}
_computeHidden(pluginStatus) {
- return !pluginStatus || pluginStatus.state !== PluginState.Failed;
+ return !pluginStatus || !isPluginErrorState(pluginStatus.state);
+ }
+
+ _getErrorText(pluginStatus) {
+ return !pluginStatus || pluginStatus.state === PluginState.Failed ?
+ 'Error: Code-owners plugin has failed' :
+ 'The code-owners plugin has configuration issue. ' +
+ 'Please contact the project owner or the host admin.';
}
_showFailDetails() {
@@ -189,7 +196,7 @@
branchState: 'LOADING',
};
}
- if (pluginStatus.state === PluginState.Failed) {
+ if (isPluginErrorState(pluginStatus.state)) {
return {
change,
branchState: 'FAILED',
diff --git a/ui/code-owners-model-loader.js b/ui/code-owners-model-loader.js
index 2fa18ac..cdc04dd 100644
--- a/ui/code-owners-model-loader.js
+++ b/ui/code-owners-model-loader.js
@@ -16,6 +16,7 @@
*/
import {SuggestionsState} from './code-owners-model.js';
+import {ServerConfigurationError} from './code-owners-api.js';
/**
* ModelLoader provides a method for loading data into the model.
@@ -39,6 +40,10 @@
try {
newValue = await propertyLoader();
} catch (e) {
+ if (e instanceof ServerConfigurationError) {
+ this.ownersModel.setServerConfigurationError(e.message);
+ return;
+ }
console.error(e);
this.ownersModel.setPluginFailed(e.message);
return;
diff --git a/ui/code-owners-model.js b/ui/code-owners-model.js
index 079eb86..ad536b4 100644
--- a/ui/code-owners-model.js
+++ b/ui/code-owners-model.js
@@ -25,9 +25,15 @@
export const PluginState = {
Enabled: 'Enabled',
Disabled: 'Disabled',
+ ServerConfigurationError: 'ServerConfigurationError',
Failed: 'Failed',
};
+export function isPluginErrorState(state) {
+ return state === PluginState.ServerConfigurationError ||
+ state === PluginState.Failed;
+}
+
export const SuggestionsType = {
BEST_SUGGESTIONS: 'BEST_SUGGESTIONS',
ALL_SUGGESTIONS: 'ALL_SUGGESTIONS',
@@ -148,6 +154,11 @@
PluginState.Enabled : PluginState.Disabled});
}
+ setServerConfigurationError(failedMessage) {
+ this._setPluginStatus({state: PluginState.ServerConfigurationError,
+ failedMessage});
+ }
+
setPluginFailed(failedMessage) {
this._setPluginStatus({state: PluginState.Failed, failedMessage});
}
@@ -163,7 +174,7 @@
return status1 === status2;
}
if (status1.state !== status2.state) return false;
- return status1.state === PluginState.Failed ?
+ return isPluginErrorState(status1.state)?
status1.failedMessage === status2.failedMessage :
true;
}
diff --git a/ui/owner-requirement.js b/ui/owner-requirement.js
index a173c75..e6ced63 100644
--- a/ui/owner-requirement.js
+++ b/ui/owner-requirement.js
@@ -18,7 +18,7 @@
import {OwnerStatus} from './code-owners-fetcher.js';
import {CodeOwnersModelMixin} from './code-owners-model-mixin.js';
import {showPluginFailedMessage} from './code-owners-banner.js';
-import {PluginState} from './code-owners-model.js';
+import {isPluginErrorState} from './code-owners-model.js';
/**
* Owner requirement control for `submit-requirement-item-code-owners` endpoint.
@@ -137,7 +137,7 @@
}
_pluginFailed(pluginStatus) {
- return pluginStatus && pluginStatus.state === PluginState.Failed;
+ return pluginStatus && isPluginErrorState(pluginStatus.state);
}
_onStatusChanged(status, userRole) {
