-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add better namespace logic to warnet logs
#659
Open
mplsgrant
wants to merge
8
commits into
bitcoin-dev-project:main
Choose a base branch
from
mplsgrant:2024-10-warnet-logs-namespaces
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Add better namespace logic to warnet logs
#659
mplsgrant
wants to merge
8
commits into
bitcoin-dev-project:main
from
mplsgrant:2024-10-warnet-logs-namespaces
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
A blank namespace is better than using "default". This is because we can figure out the proper namespace to query against within the logic of the `_logs` function or the user can specify the namespace as they desire. Also, Service Account users have no reason to query the `default` namespace.
There is also no reason to eagerly assign a namespace at the beginning of the `_logs` function because we can figure out the right namespace later on based on context. Also, we should remove the namespace from the Exception handling because we may not have a namespace at that point.
We need a way to filter out pods when the user specifies a namespace using: `warnet logs --namespace SOME_NAMESPACE` This is because an admin user may specify a namespace, and we want to show them only those pods that belong to the namespace. Performing this action in `format_pods` seems natural.
When the user simply runs `warnet logs` or `warnet logs --namespace NS`, we should handle that logic separately from when they run `warnet logs POD_NAME`. This is because doing so separates the logic of wrangling multiple namespaces. When the user runs `warnet logs` or `warnet logs --namespace NS`, the logic of wrangling the namespaces is very simple: either grab all the namespaces we can, or grab one specific namespace. When the user runs `warnet logs SOME_POD`, we find ourselves in a situation where we actually need to query all the namespaces and the whittle down the results to one namespace which contains the pod OR let the user know that SOME_POD exists in multiple namespaces -- in which case we ask them to narrow down their query to a specific namespace.
When the user runs `warnet logs SOME_POD --namespace NS`, we can actually just try to grab the pod from that namespace. This is handled in the "else" section towards the end of this commit. However, when the user does not specify a namespace (`warnet logs SOME_POD`), then we get to query all the namepaces available to the user, whittle them down, and give the user what they want. We also ask the user to specify the namespace if we find their desired pod in more than one namespace.
I noticed that I did not include a `return` if the user simply searches for a non-existent pod. So, I added the `return` statement. In order to make a test for this, I realized I would need to parse the output of `warnet logs` to search for "Traceback (" in order to see if there is some kind of error. Otherwise, if I simple "expect" a message, the test will pass even though the program errors out with some kind of stack trace. That is why I made `expect_without_traceback`. It will catch the missing `return` statement. I will add expect_without_traceback to my other pexpect statements in the next commit.
I have extended my test to use `expect_without_traceback`. This means that I swapped out each `expect` instance with that function, and in doing so, noticed that I needed to watch for `\r` since we are using inquirer and click which seemingly terminate lines with `\r` as opposed to `\n`. I also decided to use a short timeout to give the program a chance to generate a Traceback. If a Traceback is not found within 2 seconds, I then move on to check if we found our expected string and return that information as a bool. I also promoted the bitcoin "slug" string to an instance attribute. I also "demoted" the `sut` from an instance attribute to simply a variable.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
Closed
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.
This addresses #657. It includes a test, and commentary is in each commit.