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

Display formatted status during status wait #2261

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

raphink
Copy link
Member

@raphink raphink commented Jan 24, 2024

Signed-off-by: Raphaël Pinson [email protected]

@raphink raphink requested a review from a team as a code owner January 24, 2024 15:10
@raphink raphink requested a review from derailed January 24, 2024 15:10
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@raphink Thanks for this update Raphael!

status/k8s.go Outdated
@@ -357,9 +358,18 @@ func (k *K8sStatusCollector) Status(ctx context.Context) (*Status, error) {
}
if !k.statusIsReady(s) && k.params.Wait {
time.Sleep(defaults.WaitRetryInterval)
statusFmt := s.Format()
for i := 1; i < lines; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we could use a progress bar for this. Likely there might be other places that could benefit with user feedback.

We should probably use a help func to clear out the output if any.
nit: Docs might help here to indicate what this sequence does...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, the sequence is to set the cursor one line up.

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the progress bar, I'm not a big fan because:

  1. we can't really know the % progress
  2. as a user, I'd rather have details of what is going on (and see if something is stuck) in the same format as cilium status)

Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@raphink Thanks for the updates!

status/k8s.go Outdated
@@ -357,13 +358,28 @@ func (k *K8sStatusCollector) Status(ctx context.Context) (*Status, error) {
}
if !k.statusIsReady(s) && k.params.Wait {
time.Sleep(defaults.WaitRetryInterval)
if k.params.Output != "json" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the predefined const. Also perhaps best to check summary vs json in case other fmts are exposed in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The const is in a package I can't import though, because that creates a dependency loop. Should we move the const to another package?

status/k8s.go Outdated
continue
}

for i := 1; i < lines; i++ {
fmt.Print("\033[A\033[2K")
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this sequence fare on different platforms?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be pretty safe on Linux and MacOS. On Windows, it ANSI codes seem to be well supported for Windows 10+.

@raphink
Copy link
Member Author

raphink commented Feb 19, 2024

@derailed I believe I've addressed all comments!

Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@raphink Thank you for the updates Raphael!

@ti-mo
Copy link
Contributor

ti-mo commented Feb 22, 2024

I think all jobs need to be rerun, looks like there was an issue with helm.cilium.io yesterday.

@raphink
Copy link
Member Author

raphink commented Feb 22, 2024

OK, let's run again 👍🏼

@raphink
Copy link
Member Author

raphink commented Feb 22, 2024

looks like "https://helm.cilium.io/" is not a valid chart repository or cannot be reached: Get "https://helm.cilium.io/index.yaml": tls: failed to verify certificate: x509: certificate signed by unknown authority

still broken 😢

@raphink
Copy link
Member Author

raphink commented Feb 22, 2024

That's really weird though, that repo works on my machine 🤔

Signed-off-by: Raphaël Pinson <[email protected]>
Signed-off-by: Raphaël Pinson <[email protected]>
Signed-off-by: Raphaël Pinson <[email protected]>
Signed-off-by: Raphaël Pinson <[email protected]>
@michi-covalent michi-covalent merged commit aa40213 into cilium:main Feb 28, 2024
13 checks passed
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.

4 participants