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

[RHOAIENG-10311] Backend service endpoints for connection types #3048

Conversation

jeff-phillips-18
Copy link
Contributor

RHOAIENG-10311

Description

Adds backend endpoints for ConnectionType objects.
Includes GET, POST, PUSH, PATH, DELETE.
Adds a frontend service to access these endpoints.

How Has This Been Tested?

This has been tested locally by adding temporary calls in the UI

Test Impact

Tests will need to be added when the UI accesses these new functions.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

Comment on lines 15 to 44
export const fetchConnectionType = (name: string): Promise<ConfigMapKind> => {
const url = `/api/connection-types/${name}`;
return axios
.get(url)
.then((response) => response.data)
.catch((e) => {
throw new Error(e.response.data.message);
});
};

export const createConnectionType = (connectionType: ConfigMapKind): Promise<ResponseStatus> => {
const url = `/api/connection-types`;
return axios
.post(url, connectionType)
.then((response) => response.data)
.catch((e) => {
throw new Error(e.response.data.message);
});
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do the conversion between types? Do you think it makes sense that in app we always work with the parsed object. I don't know of a time when we would need to use the raw config map.

Suggested change
export const fetchConnectionType = (name: string): Promise<ConfigMapKind> => {
const url = `/api/connection-types/${name}`;
return axios
.get(url)
.then((response) => response.data)
.catch((e) => {
throw new Error(e.response.data.message);
});
};
export const createConnectionType = (connectionType: ConfigMapKind): Promise<ResponseStatus> => {
const url = `/api/connection-types`;
return axios
.post(url, connectionType)
.then((response) => response.data)
.catch((e) => {
throw new Error(e.response.data.message);
});
};
export const fetchConnectionType = (name: string): Promise<ConnectionTypeConfigMapObj> => {
const url = `/api/connection-types/${name}`;
return axios
.get(url)
.then((response) => toConnectionTypeConfigMapObj(response.data))
.catch((e) => {
throw new Error(e.response.data.message);
});
};
export const createConnectionType = (connectionType: ConnectionTypeConfigMap): Promise<ResponseStatus> => {
const url = `/api/connection-types`;
return axios
.post(url, toConnectionTypeConfigMap(connectionType))
.then((response) => response.data)
.catch((e) => {
throw new Error(e.response.data.message);
});
};

await coreV1Api.deleteNamespacedConfigMap(name, dashboardNamespace);
return { success: true, error: '' };
} catch (e) {
const error = `Unable to delete connection type: ${e.message}.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use errorHandler as we will eventually have stricter ts / eslint rules.

Suggested change
const error = `Unable to delete connection type: ${e.message}.`;
const error = `Unable to delete connection type: ${errorHandler(e)}.`;


const isConnectionTypeConfigMap = (configMap: V1ConfigMap): boolean => {
const labels = configMap.metadata.labels;
return labels[ODH_DASHBOARD_KEY] === 'true' || labels[CONNECTION_TYPE_KEY] === 'true';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't labels be undefined? Also we need both labels to be present.

Suggested change
return labels[ODH_DASHBOARD_KEY] === 'true' || labels[CONNECTION_TYPE_KEY] === 'true';
return labels?.[ODH_DASHBOARD_KEY] === 'true' && labels?.[CONNECTION_TYPE_KEY] === 'true';

Comment on lines 58 to 61
export const patchConnectionType = (
connectionType: ConnectionTypeConfigMapObj,
): Promise<ResponseStatus> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's omit this generic patch function and add a specific one that constructs a patch just for setting the enabled annotation:

             {
                op: 'replace',
                path: '/metadata/annotations/opendatahub.io~1enabled',
                value: ...,
              }

@christianvogt
Copy link
Contributor

/lgtm
/approve

Copy link
Contributor

openshift-ci bot commented Aug 2, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Aug 2, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 73e69e5 into opendatahub-io:main Aug 2, 2024
6 checks passed
@jeff-phillips-18 jeff-phillips-18 deleted the connection-type-endpoints branch November 12, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants