Add debugging dashboard url to workload creation output #48
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Obliviour
reviewed
Dec 12, 2023
Obliviour
reviewed
Dec 12, 2023
xpk.py
Outdated
|
|
||
| Args: | ||
| args: user provided arguments for running the command. | ||
|
|
||
| Returns: | ||
| str: | ||
| identifier of outlier dashbord if deployed in project, | ||
| identifier of dashbord if deployed in project, |
There was a problem hiding this comment.
nit dashbord -> dashboard
There was a problem hiding this comment.
might be misspelled in the other function descriptions as well
bvandermoon
reviewed
Dec 12, 2023
bvandermoon
approved these changes
Dec 13, 2023
Obliviour
reviewed
Dec 14, 2023
xpk.py
Outdated
| None otherwise. | ||
| """ | ||
| outlier_dashboard_filter = "displayName:'GKE - TPU Monitoring Dashboard'" | ||
| dashboard_id = get_gke_dashboard(args, outlier_dashboard_filter) |
There was a problem hiding this comment.
Overall looks great!
I see that we check the dashboard_id in two ways before erroring:
- if it is None which behind the scenes means an error in the command
- If not dashboard_id which means the dashboard id is empty and there are no dashboards available.
I think we can make this easier to read by having get_gke_dashboard return a tuple with (return_error, return_val) syntax? Otherwise if dashboard_id is None and if not dashboard_id are hard to tell what they are checking. Like for example if not dashboard_id would be True if dashboard_id is None. What do you think about this?
Obliviour
reviewed
Dec 14, 2023
xpk.py
Outdated
| args: user provided arguments for running the command. | ||
|
|
||
| Returns: | ||
| bool: |
There was a problem hiding this comment.
I think get_gke_outlier_dashboard only returns a str
Obliviour
approved these changes
Dec 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes / Features
cloud-tpu-diagnosticspackage.Testing / Documentation
Testing details.