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

OSSM: General Improvements #1958

Closed
cam-garrison opened this issue Oct 11, 2023 · 1 comment · Fixed by #1959
Closed

OSSM: General Improvements #1958

cam-garrison opened this issue Oct 11, 2023 · 1 comment · Fixed by #1959
Assignees
Labels
feature/ossm OpenShift Service Mesh kind/story A user story for larger work. Should always be referenced by a "tracker" labelled issue.

Comments

@cam-garrison
Copy link

cam-garrison commented Oct 11, 2023

Creating this issue to encompass several pieces of review + suggestions from the original PR #1088 which were not included in the PR.

          I suggest adding the same comment as we have present in the frontend code.
/**
 * Feature flags are required in the config -- but upgrades can be mixed and omission of the property
 * usually ends up being enabled. This will prevent that as a general utility.
 */
export const featureFlagEnabled = (disabledSettingState?: boolean): boolean =>
  disabledSettingState === false;

Originally posted by @christianvogt in #1088 (comment)

          ? getRoute(notebookName, projectName).then((route) => route.spec.host)

Originally posted by @christianvogt in #1088 (comment)

          Should we fall back to retrieving the host through the route?

By enabling the flag without service mesh, we'll get an invalid url.

    if (enableServiceMesh) {
      host = await getServiceMeshGwHost(fastify, namespace).catch((e) => {
        fastify.log.warn(`Failed getting service mesh route ${notebookName}: ${e.message}`);
        return undefined;
      });
    }
    if (!host) {
      const route = await getRoute(fastify, namespace, notebookName).catch((e) => {
        fastify.log.warn(`Failed getting route ${notebookName}: ${e.message}`);
        return undefined;
      });
      host = route?.spec.host;
    }

Originally posted by @christianvogt in #1088 (comment)

@andrewballantyne
Copy link
Member

PR closed

@github-project-automation github-project-automation bot moved this from Dev To do to Done in ODH Dashboard Planning Nov 3, 2023
@andrewballantyne andrewballantyne added the kind/story A user story for larger work. Should always be referenced by a "tracker" labelled issue. label Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/ossm OpenShift Service Mesh kind/story A user story for larger work. Should always be referenced by a "tracker" labelled issue.
Projects
Status: Done
Status: No status
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants