Skip to content

upgrade tablewriter #4026

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

upgrade tablewriter #4026

wants to merge 2 commits into from
+382 −240

Conversation

cpanato
Copy link
Member

@cpanato cpanato commented Jun 3, 2025

What type of PR is this?

/kind cleanup
/kind feature

What this PR does / why we need it:

  • upgrade tablewriter

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

upgrade tablewriter lib

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Jun 3, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from jimangel and jrsapi June 3, 2025 10:42
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 3, 2025
@cpanato
Copy link
Member Author

cpanato commented Jun 3, 2025

/assign @xmudrii @Verolop @puerco
cc @kubernetes/release-engineering

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2025
@cpanato cpanato force-pushed the update-tablewriter branch from ed5e43f to d130c43 Compare June 5, 2025 08:05
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 5, 2025
@cpanato
Copy link
Member Author

cpanato commented Jun 5, 2025

/assign @xmudrii @puerco @saschagrunert @ameukam

xmudrii
xmudrii requested changes Jun 5, 2025
View reviewed changes
cmd/schedule-builder/cmd/markdown.go

// Check if the next patch release is in the Previous Patch list, if yes dont read in the output
if !patchReleaseInPreviousList(releaseSchedule.Next.Release, releaseSchedule.PreviousPatches) {
table.Append([]string{strings.TrimSpace(releaseSchedule.Next.Release), strings.TrimSpace(releaseSchedule.Next.CherryPickDeadline), strings.TrimSpace(releaseSchedule.Next.TargetDate), ""})
_ = table.Append([]string{strings.TrimSpace(releaseSchedule.Next.Release), strings.TrimSpace(releaseSchedule.Next.CherryPickDeadline), strings.TrimSpace(releaseSchedule.Next.TargetDate), ""})
Copy link
Member

Choose a reason for hiding this comment

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

Why do we completely ignore the error? We should at least log it so we know that there's something wrong going on, otherwise we might just not notice that we're missing some data.

Copy link
Member

Choose a reason for hiding this comment

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

Same in other places where this is present

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 was already ignoring the error, but will add a log on that

pkg/gcp/build/build.go
@@ -404,14 +406,27 @@ func ListJobs(project string, lastJobs int64) error {
return fmt.Errorf("failed to listing the builds: %w", err)
}

table := tablewriter.NewWriter(os.Stdout)
table.SetHeader([]string{"Start Time", "Finish Time", "Status", "Console Logs"})
table := tablewriter.NewTable(os.Stdout,
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some function in release-utils that's going to return a table that's already configured with everything, so that we don't have to write this much code every time we want to use a table? Does that make sense at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

sg

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2025
@cpanato cpanato mentioned this pull request Jul 8, 2025
Merged
cpanato added 2 commits July 17, 2025 13:47
@cpanato
upgrade github.com/olekukonko/tablewriter and other deps
e169c8e 
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
@cpanato
migrate to the new table configs
52483f8 
Signed-off-by: Carlos Panato <ctadeu@gmail.com>

more fixes

Signed-off-by: Carlos Panato <ctadeu@gmail.com>

fix lints

Signed-off-by: Carlos Panato <ctadeu@gmail.com>

more fixes

Signed-off-by: Carlos Panato <ctadeu@gmail.com>

revert upgrade for now
@cpanato cpanato force-pushed the update-tablewriter branch from d130c43 to 52483f8 Compare July 17, 2025 14:42
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2025
@xmudrii
Copy link
Member

xmudrii commented Jul 18, 2025

@cpanato Do we want to merge this now or wait for kubernetes-sigs/release-utils#139?

@cpanato
Copy link
Member Author

cpanato commented Jul 18, 2025

wait for the next

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2025
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@cpanato
Copy link
Member Author

cpanato commented Aug 12, 2025

we can close this

@cpanato cpanato closed this Aug 12, 2025
@cpanato cpanato deleted the update-tablewriter branch August 12, 2025 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xmudrii xmudrii xmudrii requested changes

@jimangel jimangel Awaiting requested review from jimangel

@jrsapi jrsapi Awaiting requested review from jrsapi

Assignees

@saschagrunert saschagrunert

@Verolop Verolop

@ameukam ameukam

@puerco puerco

@xmudrii xmudrii

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. needs-priority needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/release Categorizes an issue or PR as relevant to SIG Release. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cpanato @k8s-ci-robot @xmudrii @saschagrunert @Verolop @ameukam @puerco