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

[Service Mesh]: handle istio proxy events #1937

Merged

Conversation

cam-garrison
Copy link

Closes: #1935

Description

This PR adds displaying the various event states when pulling the istio-proxy container. Before this PR, the percentage is not updated for the different stages of pulling the istio-proxy container as it was for the oauth-proxy container.

This is implemented by adding a function wrapping around the switch statement previously used for oauth-proxy, and now can be used for either oauth or istio proxy depending on which is being pulled/started. The helper function capitalizeFirstLetter is used to maintain the same messages as before (in terms of capitalizing Oauth vs oauth).

Note: the 'pulling istio proxy' message should not appear - this proxy sidecar will be pulled for the dashboard's sidecars as well so.

How Has This Been Tested?

Tested by installing operator v2 with service mesh: see opendatahub-io/opendatahub-operator#605. Then, tested creating notebooks from the jupyter tile and from a data science project to ensure that the "Istio proxy container started" message is shown last. Also, tested with ODHDashboardConfig.disableServiceMesh=true in both cases to see that Oauth related event messages are still shown.

Test Impact

I am not sure where to add tests for this kind of a change - please point me in the right direction if testing would be needed for this PR!

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 tests & storybook for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot requested review from Gkrumbach07 and pnaik1 October 9, 2023 20:43
@cam-garrison cam-garrison linked an issue Oct 9, 2023 that may be closed by this pull request
@cam-garrison
Copy link
Author

@andrewballantyne could you take a look at this when you have some bandwidth?

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

If this feature has a path to RHOAI, I need you to clean up some of this (which I can give you direction on). Waiting for your response on slack or here.

/hold

For now let's just hold up this change.

@openshift-ci openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Nov 3, 2023
Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Dang -- these kind of PRs remind me all the tech debt we need to do to get away from the Jupyter Tile 😞 -- not your problem though. Can you just update the code to be less open-ended and then we can go ahead with this (do it in both files please).

/approve

Just made the code changes and we can lgtm this.

frontend/src/pages/projects/notebook/utils.ts Outdated Show resolved Hide resolved
@andrewballantyne
Copy link
Member

/unhold

It's not awesome, but there is little to do here other than get started on that refactor to get rid of the custom Jupyter tile logic (which is really in most cases duplicate of our Projects view)

@openshift-ci openshift-ci bot removed the do-not-merge/hold This PR is hold for some reason label Nov 17, 2023
@andrewballantyne
Copy link
Member

Please do to both areas @cam-garrison & squash commits into a single commit always. Rebase your PR if you need to, never merge to save you the headache when squashing.

Let me know if you have any questions about this.

Co-authored-by: Andrew Ballantyne <[email protected]>
Copy link
Contributor

openshift-ci bot commented Dec 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

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-merge-bot openshift-merge-bot bot merged commit 51d5cfb into opendatahub-io:f/ossm Dec 7, 2023
3 checks passed
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.

[Feature Request]: Support Istio Proxy notebook event states
2 participants