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

feat: Rewrite the Tensorboards Web App charm following the sidecar pattern #74

Merged
merged 28 commits into from
Jun 28, 2023

Conversation

orfeas-k
Copy link
Contributor

@orfeas-k orfeas-k commented Jun 13, 2023

This PR rewrites the Tensorboards Web App (TWA) charm following the sidecar pattern
In short:

  • Update pip packages' versions (bumped SDI version in TWA charm)
  • Update README.md file to match the charm where it is located
  • Introduce CONTRIBUTING.md file
  • Add upstream manifests as Jinja2 templates
  • Set charm's status to blocked when there is no ingress relation
  • Rewrite the charm following the sidecar pattern
  • Adjust joint charms' deploy test to match changes
  • Add integration tests for TWA charm
  • Run TWA integration tests in Github CI
  • Drop TWA container port from config option
  • Use production as default for config's Backend-mode

Upgrade instructions

  1. Trust the charm
juju trust tensorboards-web-app
  1. Refresh the charm providing:
    i. the image since oci-image was changed to tensorboards-web-app-image.
    ii. the path to the local charm or the corresponding channel (for charms released in charmhub)
juju refresh tensorboards-web-app --resource tensorboards-web-app-image=kubeflownotebookswg/tensorboards-web-app:v1.7.0 --path="./tensorboards-web-app_ubuntu-version-arch.charm" 

or

juju refresh tensorboards-web-app --resource tensorboards-web-app-image=kubeflownotebookswg/tensorboards-web-app:v1.7.0 --channel=track/risk

@orfeas-k orfeas-k requested a review from a team as a code owner June 13, 2023 15:25
@beliaev-maksim beliaev-maksim marked this pull request as draft June 13, 2023 15:44
@orfeas-k orfeas-k changed the title WIP: feat: Rewrite the Tensorboards Web App charm following the sidecar pattern feat: Rewrite the Tensorboards Web App charm following the sidecar pattern Jun 14, 2023
@orfeas-k orfeas-k marked this pull request as ready for review June 14, 2023 12:15
@orfeas-k orfeas-k closed this Jun 15, 2023
@orfeas-k orfeas-k deleted the orfeas-twa-rewrite-sidecar branch June 15, 2023 08:34
@orfeas-k orfeas-k restored the orfeas-twa-rewrite-sidecar branch June 15, 2023 08:35
@orfeas-k orfeas-k reopened this Jun 15, 2023
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @orfeas-k, some comments.

charms/tensorboards-web-app/CONTRIBUTING.md Show resolved Hide resolved
charms/tensorboards-web-app/CONTRIBUTING.md Outdated Show resolved Hide resolved
charms/tensorboards-web-app/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/workflows/integrate.yaml Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
@ca-scribner
Copy link
Contributor

Extremely pedantic suggestion for the upgrade instructions :)

juju refresh tensorboards-web-app ... --channel=version/mode

should be

juju refresh tensorboards-web-app ... --channel=track/risk

Doesn't change the function, but gets the terminology consistent with elsewhere.

Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

Some suggested changes, but overall looks really good!

charms/tensorboards-web-app/CONTRIBUTING.md Outdated Show resolved Hide resolved
charms/tensorboards-web-app/requirements-integration.in Outdated Show resolved Hide resolved
charms/tensorboards-web-app/requirements.in Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Show resolved Hide resolved
charms/tensorboards-web-app/tests/unit/test_operator.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/tests/unit/test_operator.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/tests/unit/test_operator.py Outdated Show resolved Hide resolved
.github/workflows/integrate.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

whoops. misclick on the previous review!

orfeas-k added a commit to canonical/kubeflow-roles-operator that referenced this pull request Jun 19, 2023
Remove tensorboards-web-app aggregation roles since TWA charm now
follows the sidecar pattern with Pebble and can apply aggregation roles
itself. See canonical/kubeflow-tensorboards-operator#74
ca-scribner
ca-scribner previously approved these changes Jun 23, 2023
- Modify on_remove to use krh.delete() function
- Rename err to error in all instances
- Remove container connection check since it is done by update_layer()
- Set status in _on_event instead of _configure_mesh()
- Stop logging inside check_leader
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Some comments @orfeas-k, after that I think I can approve.

charms/tensorboards-web-app/src/charm.py Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Show resolved Hide resolved
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @orfeas-k , great initial contribution!

@orfeas-k orfeas-k merged commit bc445de into main Jun 28, 2023
12 checks passed
@orfeas-k orfeas-k deleted the orfeas-twa-rewrite-sidecar branch June 28, 2023 12:58
orfeas-k added a commit to canonical/kubeflow-roles-operator that referenced this pull request Jun 28, 2023
Remove tensorboards-web-app aggregation roles since TWA charm now
follows the sidecar pattern with Pebble and can apply aggregation roles
itself. See canonical/kubeflow-tensorboards-operator#74
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.

6 participants