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

Add namespaces to mirrord ls output #3003

Conversation

Razz4780
Copy link
Contributor

@Razz4780 Razz4780 commented Jan 3, 2025

Issue #2999

Since mirrord ls currently returns a list of target paths, it was not possible to implement this without adding an extra env. IDEs will have to inspect mirrord binary version or try parsing the new format with fallback to the old one.

Additionally, fixed mirrord::port_forward module visibility and removed some dead code.

@Razz4780 Razz4780 requested a review from meowjesty January 7, 2025 15:05
Copy link
Member

@meowjesty meowjesty left a comment

Choose a reason for hiding this comment

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

Some nits here and there.

The main thing is, I would prefer to have kube interactions under the kube resource struct, as I find it's easier to think about (my thought process is usually: I want kube stuff, create this struct, now it offers me an api with exactly what I want ... Oh it doesn't have what I want? I add it to the struct). But we never talked about api design (not that I remember), maybe we should bring this up to the team?

mirrord/cli/src/list.rs Show resolved Hide resolved
mirrord/cli/src/list.rs Show resolved Hide resolved
mirrord/kube/src/api/kubernetes/list.rs Outdated Show resolved Hide resolved
mirrord/cli/src/list.rs Outdated Show resolved Hide resolved
mirrord/cli/src/list.rs Show resolved Hide resolved
mirrord/cli/src/list.rs Outdated Show resolved Hide resolved
Copy link
Member

@meowjesty meowjesty left a comment

Choose a reason for hiding this comment

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

And here you prefer present for all the docs. I feel trolled.

🧌

@Razz4780 Razz4780 added this pull request to the merge queue Jan 8, 2025
Merged via the queue into metalbear-co:main with commit 2ec5ab3 Jan 8, 2025
17 checks passed
@Razz4780 Razz4780 deleted the michals/mbe-636-add-namespaces-to-mirrord-ls-output branch January 8, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants